Improve annotations #1343

tonyhallett · 2021-05-08T16:27:40Z

Description

I have created a new function that is used for all the decorators that are applicable to parameters and properties ( including accessors ).

//e.g
function multiInject(serviceIdentifier: interfaces.ServiceIdentifier<any>) {
  return createTaggedDecorator(new Metadata(METADATA_KEY.MULTI_INJECT_TAG, serviceIdentifier));
}

function createTaggedDecorator(
    metadata:interfaces.MetadataOrMetadataArray,
    callback?:(target: any, targetKey: string, index?: number | PropertyDescriptor) => void
) {
    return function(target: any, targetKey: string, index?: number | PropertyDescriptor) {
        if(callback){
            callback(target, targetKey, index);
        }
        if (typeof index === "number") {
            tagParameter(target, targetKey, index, metadata);
        } else {
            tagProperty(target, targetKey, metadata);
        }
    };
}

I have also exported createTaggedDecorator from inversify.ts to be available for anyone to use.
Note that this function allows multiple metadata to be provided through a single decorator.

I have also refactored _tagParameterOrProperty as it was checking if it was being applied for a parameter or a property. Now it is passed only the information that it requires.

I also changed the type to include undefined
let paramOrPropertyMetadata: interfaces.Metadata[] | undefined = paramsOrPropertiesMetadata[key];
and then checked against undefined.

Related Issue

#1342

Motivation and Context

#1342

How Has This Been Tested?

Properly

Updated docs / Refactor code / Added a tests case (non-breaking change)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have updated the changelog.

…s on setters

src/inversify.ts

src/annotation/inject.ts

tonyhallett

Perhaps could permit inject and multi inject to have two addditional parameters as a shortcut for the named and tagged decorators.

notaphplover

It looks great, I just added some comments

src/utils/js.ts

src/inversify.ts

notaphplover

Great, we only need to update the changelog and this PR should be ready :)

notaphplover

Great! I just added some comments :)

src/annotation/decorator_utils.ts

tonyhallett · 2021-05-09T12:34:00Z

I have one other commit to come regarding an unhelpful error message that can arise for decorated properties missing inject / multi inject.

…ators

src/interfaces/interfaces.ts

PodaruDragos · 2021-05-10T16:35:44Z

@tonyhallett that introduces another problem for eslint.
I know we can disable some rules to our fitting but using Object will throw this error:

Don't use `Object` as a type. The `Object` type actually means "any non-nullish value", so it is marginally better than `unknown`.
- If you want a type meaning "any object", you probably want `Record<string, unknown>` instead.
- If you want a type meaning "any value", you probably want `unknown` instead

link

notaphplover

What should we do with


    export interface MetadataReader {
        getPropertiesMetadata(constructorFunc: Function): MetadataMap;
    }

Given that symbols can be keys ?

I would not change it. It's something Typescript has to fix in order to allow symbol index signatures

Should the wiki have an example with symbol property injection ?

Probably, but that's something we could add in another PR

Are you happy with the name of the additional Target property identifier ?

I'm not sure, do we need it? Maybe I'm missing something

src/planning/target.ts

notaphplover · 2021-05-10T18:20:39Z

src/annotation/inject_base.ts

+
+export function injectBase(metadataKey:string){
+  return (serviceIdentifier: ServiceIdentifierOrFunc) => {
+    return (target: Object,


I think Record<string | symbol, unknown> | interfaces.Newable<unknown> is a better type for target

Not acceptable by the compiler

is not assignable to type 'Record<string | symbol, unknown>'. Index signature is missing in type

I think that I have provided a solution

interface ConstructorFunction { new(...args:any[]):unknown, readonly prototype:unknown } interface Prototype { constructor: Function; } export type DecoratorTarget = Prototype | ConstructorFunction;

It's indeed a valid solution. We want to introduce some linter rules in the future and that's the reason to ban some types. If you want I can create a PR to your branch with equivalent types which respect this rule :)

Can you elaborate on CI ?

By all means change the typing. I hadn't seen the banning of Function. I look forward to seeing the equivalent types.

we can also just add this into out tslint setup right now and then just try out what works best, since ban-types are somehow related to both linters.

What was the problem with Record<string, unknown anyway ?

What was the problem with Record<string, unknown anyway ?

When applied to a property typescript will complain

is not assignable to type 'Record<string | symbol, unknown>'. Index signature is missing in type CLASSNAME

This is why

class ClassWithDecorators { constructor(@decorator param:Dependency){} @decorator //@ts-ignore prop:Dependency @decorator static staticProp:Dependency } // here is the error const typescriptError:Record<string,unknown> = ClassWithDecorators.prototype

the js

var ClassWithDecorators = /** @class */ (function () { function ClassWithDecorators(param) { } __decorate([ decorator, __metadata("design:type", Dependency) ], ClassWithDecorators.prototype, "prop", void 0); __decorate([ decorator, __metadata("design:type", Dependency) ], ClassWithDecorators, "staticProp", void 0); ClassWithDecorators = __decorate([ __param(0, decorator), __metadata("design:paramtypes", [Dependency]) ], ClassWithDecorators); return ClassWithDecorators; }());

They differ. object is allowed in tslint.

https://github.com/typescript-eslint/typescript-eslint/blob/1c1b572c3000d72cfe665b7afbada0ec415e7855/packages/eslint-plugin/src/rules/ban-types.ts#L99

https://github.com/palantir/tslint/blob/285fc1db18d1fd24680d6a2282c6445abf1566ee/src/configs/recommended.ts#L23

@PodaruDragos
tell eslint to ignore the object and the Function types where they are used. The errors is there for lazy typing and errors that arise because of that.
In this instance the type signatures are ok and definitely better than unknown and of couse any.

If the class being decorated had
[key:string]:unknown
Then Record<string | symbol, unknown> | interfaces.Newable<unknown> will not error

tonyhallett

Please see #1343 (comment).

src/annotation/decorator_utils.ts

tonyhallett · 2021-05-13T08:52:05Z

src/annotation/decorator_utils.ts

+    prototype:T
+}
+
+export type DecoratorTarget<T = Object> = ConstructorFunction<T> |  T


I believe that this is the best way of typing the target.
We differentiate between constructor function and prototype to prevent static properties and the prototype property is the proper way of doing so.
_tagParameterOrProperty requires annotationTarget: Function ( this cannot be typed better with generics ) and when target is a constructor function it is obviously so. Although it is through a cast in tagParameter _tagParameterOrProperty(METADATA_KEY.TAGGED, annotationTarget as ConstructorFunction, parameterIndex.toString(), metadata); the type predicate could have been used instead.

notaphplover · 2021-05-13T08:56:12Z

src/annotation/decorator_utils.ts

+
+interface ConstructorFunction<T = Object>{
+    new (...args:unknown[]): T,
+    prototype:T


I think prototype should not be T since T is not an instance of the type, but the prototype instead

var ClassWithDecorators = /** @class */ (function () { function ClassWithDecorators(param) { } __decorate([ decorator, __metadata("design:type", Dependency) ], ClassWithDecorators.prototype, "prop", void 0); __decorate([ decorator, __metadata("design:type", Dependency) ], ClassWithDecorators, "staticProp", void 0); ClassWithDecorators = __decorate([ __param(0, decorator), __metadata("design:paramtypes", [Dependency]) ], ClassWithDecorators); return ClassWithDecorators; }());

Mapped T to Prototype<T>, properties now optional.

type Prototype<T> = { [Property in keyof T ]: T[Property] extends Function? T[Property] : T[Property] | undefined } & {constructor:Function} interface ConstructorFunction<T = Object>{ new (...args:unknown[]): T, prototype:Prototype<T> } export type DecoratorTarget<T = Object> = ConstructorFunction<T> | Prototype<T>

src/annotation/decorator_utils.ts

Co-authored-by: notaphplover <roberto.pintos.lopez@gmail.com>

PodaruDragos

This is definitely better ( nice work). feel free to merge when able. LGTM.

tonyhallett · 2021-05-13T17:37:43Z

@PodaruDragos
@notaphplover

Thanks for your contributions on this and other pull requests !

Add this to the changelog as exports the helper createTaggedDecorator ?

Are we doing merge commits or rebase and merge ?

tonyhallett added 9 commits August 24, 2019 11:53

Merge branch 'master' of https://github.com/inversify/InversifyJS

7b062e2

Merge branch 'master' of https://github.com/inversify/InversifyJS

2cf47bf

Merge branch 'master' of https://github.com/inversify/InversifyJS

36310b7

Merge branch 'master' of https://github.com/inversify/InversifyJS

57ad651

Merge branch 'master' of https://github.com/inversify/InversifyJS

9ec5940

Merge branch 'master' of https://github.com/inversify/InversifyJS

ce54f28

refactor to helper createTaggedDecorator which supports all decorator…

2073070

…s on setters

make createTaggedDecorator permit multiple metadata

035d68c

refactor tagParameter and tagProperty

2e1c863

tonyhallett requested a review from notaphplover May 8, 2021 16:27

tonyhallett added 4 commits May 8, 2021 18:42

extract _ensureNoMetadataKeyDuplicates

dd92dc7

change parameter name to indexOrPropertyDescriptor

08e46e6

grammar correct historic test names

ff16c9a

use sinon sandbox for spying

17a0fa7

tonyhallett commented May 8, 2021

View reviewed changes

src/inversify.ts Show resolved Hide resolved

src/annotation/inject.ts Outdated Show resolved Hide resolved

tonyhallett commented May 8, 2021

View reviewed changes

notaphplover reviewed May 8, 2021

View reviewed changes

src/utils/js.ts Outdated Show resolved Hide resolved

src/utils/js.ts Outdated Show resolved Hide resolved

src/inversify.ts Show resolved Hide resolved

tonyhallett added 2 commits May 9, 2021 09:29

review

3fcb065

wiki

2e5a011

tonyhallett marked this pull request as ready for review May 9, 2021 09:09

notaphplover reviewed May 9, 2021

View reviewed changes

src/annotation/decorator_utils.ts Outdated Show resolved Hide resolved

src/annotation/decorator_utils.ts Outdated Show resolved Hide resolved

improve typing

6da7ac6

tonyhallett added 3 commits May 9, 2021 14:08

improve decorator typing

a0eede0

throw error for decorated property missing inject / multiInject decor…

c9487b8

…ators

refactor code climate

44c43f0

tonyhallett commented May 9, 2021

View reviewed changes

src/interfaces/interfaces.ts Show resolved Hide resolved

tonyhallett added 2 commits May 9, 2021 16:19

permit injection for symbol property key

2e4119f

move symbol tests to container

ee81df3

replace if condition

3137069

notaphplover reviewed May 10, 2021

View reviewed changes

src/planning/target.ts Outdated Show resolved Hide resolved

notaphplover reviewed May 10, 2021

View reviewed changes

tonyhallett and others added 5 commits May 11, 2021 10:53

DecoratorTarget type and throw for annotating static properties

0fc9b09

DecoratorTarget for js decorate

b7b7741

add test for js inject for property

9c39f67

extract getSymbolDescription

2756407

refactor: simplify types

b325f59

notaphplover self-requested a review May 12, 2021 14:23

notaphplover approved these changes May 12, 2021

View reviewed changes

tonyhallett commented May 12, 2021

View reviewed changes

src/annotation/decorator_utils.ts Show resolved Hide resolved

_tagParameterOrProperty on constructor function

39f3913

notaphplover approved these changes May 13, 2021

View reviewed changes

DecoratorTarget type

5a4462e

tonyhallett commented May 13, 2021

View reviewed changes

notaphplover reviewed May 13, 2021

View reviewed changes

src/annotation/decorator_utils.ts Outdated Show resolved Hide resolved

notaphplover reviewed May 13, 2021

View reviewed changes

src/annotation/decorator_utils.ts Show resolved Hide resolved

tonyhallett and others added 2 commits May 13, 2021 10:44

type predicate cast to type testing for

b266fe1

Co-authored-by: notaphplover <roberto.pintos.lopez@gmail.com>

Prototype type - map properties to include undefined

bf99ac1

PodaruDragos approved these changes May 13, 2021

View reviewed changes

tonyhallett added 2 commits May 14, 2021 11:47

Merge branch 'master' into improve-annotations

03e7bf9

update changelog

a2fac9b

tonyhallett merged commit 4332cb8 into inversify:master May 14, 2021

tonyhallett mentioned this pull request May 14, 2021

only inject decorator can be applied to setters #1342

Closed

tonyhallett deleted the improve-annotations branch May 14, 2021 11:04

snyk-io bot mentioned this pull request Sep 13, 2024

[Snyk] Upgrade: , semver, , arch, fs-extra, glob, inversify, jsonc-parser, minimatch, reflect-metadata, rxjs, tmp, untildify, vscode-debugprotocol, which, winreg, xml2js Organic-Hemp-mexico/pythonVSCode#1

Open

snyk-io bot mentioned this pull request Sep 22, 2024

[Snyk] Upgrade: , semver, , arch, fs-extra, glob, inversify, jsonc-parser, minimatch, reflect-metadata, rxjs, tmp, untildify, vscode-debugprotocol, which, winreg, xml2js Organic-Hemp-mexico/pythonVSCode#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve annotations #1343

Improve annotations #1343

tonyhallett commented May 8, 2021 •

edited

Loading

tonyhallett left a comment

notaphplover left a comment

notaphplover left a comment •

edited

Loading

notaphplover left a comment

tonyhallett commented May 9, 2021

PodaruDragos commented May 10, 2021

notaphplover left a comment

notaphplover May 10, 2021

tonyhallett May 11, 2021

tonyhallett May 11, 2021

notaphplover May 11, 2021 •

edited

Loading

tonyhallett May 11, 2021

PodaruDragos May 12, 2021

tonyhallett May 12, 2021 •

edited

Loading

tonyhallett May 12, 2021 •

edited

Loading

tonyhallett May 12, 2021

tonyhallett May 12, 2021

tonyhallett left a comment

tonyhallett May 13, 2021

notaphplover May 13, 2021

tonyhallett May 13, 2021

tonyhallett May 13, 2021

tonyhallett May 13, 2021

PodaruDragos left a comment

tonyhallett commented May 13, 2021

Improve annotations #1343

Improve annotations #1343

Conversation

tonyhallett commented May 8, 2021 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Checklist:

tonyhallett left a comment

Choose a reason for hiding this comment

notaphplover left a comment

Choose a reason for hiding this comment

notaphplover left a comment • edited Loading

Choose a reason for hiding this comment

notaphplover left a comment

Choose a reason for hiding this comment

tonyhallett commented May 9, 2021

PodaruDragos commented May 10, 2021

notaphplover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notaphplover May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyhallett May 12, 2021 • edited Loading

Choose a reason for hiding this comment

tonyhallett May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyhallett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PodaruDragos left a comment

Choose a reason for hiding this comment

tonyhallett commented May 13, 2021

tonyhallett commented May 8, 2021 •

edited

Loading

notaphplover left a comment •

edited

Loading

notaphplover May 11, 2021 •

edited

Loading

tonyhallett May 12, 2021 •

edited

Loading

tonyhallett May 12, 2021 •

edited

Loading