-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add decorator factories and refactor into @loopback/metadata
#775
Conversation
c6ba39d
to
c29588b
Compare
2f085bd
to
97ec9e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced the amount of code in the factory classes is worth the savings in the places where we are using these decorators. Having said that, if other team members think this is a useful addition, then I can live with that.
I checked updates in existing code only superficially, they looks mostly good except the decorator type casts - our design should not need users to do that! (See my comment below.)
As for the implementation of the factories, I find the code difficult to read because there is so much duplication! Could you please look into ways how to reuse some parts of the code, perhaps using a Strategy or Template patterns, or maybe by leveraging method decorator implementation from inside parameter decorator to have a single place that's dealing with existence/non-existence of method-level metadata?
* @param spec Metadata object from the decorator function | ||
* @param allowInheritance Inheritance will be honored, default to `true` | ||
*/ | ||
static getDecorator<D extends DecoratorType>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getDecorator
creates the impression that an existing decorator function is returned. Can we use createDecorator
instead? I think that's the usual convention in Factory Design Pattern.
@@ -41,7 +48,7 @@ export function model(definition?: ModelDefinitionSyntax) { | |||
modelDef.addProperty(p, propertyMap[p]); | |||
} | |||
|
|||
target.definition = modelDef; | |||
(<any>target).definition = modelDef; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the type of target
to Function & {definition?: ModelDefinition}
so that this cast is not needed?
Reflector.defineMetadata(RELATION_KEY, definition, target, key); | ||
}; | ||
// Apply relation definition to the model class | ||
return <PropertyDecorator>PropertyDecoratorFactory.getDecorator( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have to cast the result of PropertyDecoratorFactory.getDecorator
to PropertyDecorator
? Since we are calling PropertyDecoratorFactory
, there should have been already enough type information for tsc
to let it know that getDecorator
is returning a PropertyDecorator
.
I think this points out a flaw in our factory design.
97ec9e8
to
5b85f9f
Compare
Two more thoughts:
I'd like the first point to be addressed as part of this pull request, the second point can be deferred after this patch is landed. Thoughts? |
5b85f9f
to
41c4d33
Compare
@bajtos Thank you for the comments. I cleaned up the code to address some of your concerns. PTAL. A few points:
|
41c4d33
to
14a5a41
Compare
@bajtos I also added a commit to create |
T, | ||
M extends T | MetadataMap<T> | MetadataMap<T[]>, | ||
D extends DecoratorType | ||
>(key: string, spec: T, allowInheritance: boolean = true): D { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick:
allowInheritance: boolean = true
not sure will we have other options like allowInheritance
in the future, how about making the third parameter an option object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds good to me.
} | ||
|
||
/** | ||
* Set a reference to target for a given spec if the it's an object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set a reference to target for a given spec if it's an object
Not sure what this was meant to say, TBH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would definitely solve the problem of determining which servers a controller is meant to provide routes for.
Code coverage has dropped quite a bit by percentage, though. Add tests to the metadata package itself, then LGTM
3227ee1
to
f1c8cf9
Compare
That's a good point 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks much better now, thank you for the update @raymondfeng!
Few more comments:
-
I think we should remove
Reflector
from the exports of@loopback/metadata
and@loopback/context
to prevent people from using them. IMO, this is in line with what you wrote earlier about seeing people using Reflector incorrectly. If Reflector is difficult to use right, then we should hide it from LoopBack developers and users as much as we can. Since this change can be disruptive, I am ok to do it after this PR has landed. -
Our file name convention is to add suffix to test files,
.test.ts
for unit tests,.integration.ts
for integration tests, etc. Please apply this convention to the files you are adding in this pull request.
packages/context/src/inject.ts
Outdated
); | ||
propDecorator(target, propertyKey!); | ||
} else { | ||
// It won't happen here as `@inject` is not compatible with ClassDecorator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please throw an error here. While this code may not be called from a TypeScript codebase, I believe it's still possible to call this decorator manually from JavaScript code base. Even if it wasn't, it still a good practice to throw an error if the program ever reaches a code/branch that we did not anticipate.
packages/context/src/inject.ts
Outdated
return Reflector.getMetadata(PARAMETERS_KEY, target) || []; | ||
} | ||
method = method || ''; | ||
const meta = Reflector.getMetadata(PARAMETERS_KEY, target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, describeInjectedArguments
is reading metadata set by @inject
, which uses ParameterDecoratorFactory.createDecorator
internally.
I'd like this code to be decoupled from Reflector
and use a helper method from @loopback/metadata
to read the information stored by ParameterDecoratorFactory.createDecorator
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think MetadataInspector
is the tool to use here?
packages/context/src/inject.ts
Outdated
if (!obj) break; | ||
} | ||
const metadata: {[name: string]: Injection} = | ||
Reflector.getMetadata(PROPERTIES_KEY, target) || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here - can we use a (possibly new) helper from @loopback/metadata
to access property-level information?
Also the old code was recursing through the prototype chain while this new code is not, does it mean that Reflector.getMetadata
is taking care of that already? I would expect a flag to control whether prototypes are consulted for metadata. If there is one (and is set to true by default), then please be more explicit here and set the flag to that default value, to make the intent more obvious to people reading this code for the first time.
packages/metadata/package.json
Outdated
"devDependencies": { | ||
"@loopback/build": "^4.0.0-alpha.6", | ||
"@loopback/testlab": "^4.0.0-alpha.15", | ||
"@types/bluebird": "^3.5.18", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, bluebird
is not used by this package - could you please remove?
packages/metadata/package.json
Outdated
"@types/bluebird": "^3.5.18", | ||
"@types/debug": "0.0.30", | ||
"@types/lodash": "^4.14.87", | ||
"bluebird": "^3.5.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, bluebird
is not used by this package - could you please remove?
/** | ||
* A symbol to reference the target of a decoration | ||
*/ | ||
static TARGET = Symbol('decorationTarget'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not be using Symbols and use strings instead. Here is the use case where Symbols don't work:
-
There are two modules
EXT
andAPP
, each depending on a different version of@loopback/metadata
. As a result, there are two copies of@loopback/metadata
innode_modules
tree. As a result, there are twoTARGET
symbols inside the Node.js process, one symbol is used by EXT and another symbol is used by APP. -
APP module sets metadata using its
TARGET
symbol. -
EXT module wants to read metadata set by APP. Because it has different
TARGET
symbol, it cannot read information set by APP.
return target.length; | ||
} else { | ||
// target[member] is a function | ||
return (<any>target)[member!].length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps {[methodName: string]: Function}
would be a more appropriate type than any
? I think the type of "a n object with arbitrary methods" may be useful in other places too and therefore is worth sharing via an explicit type
. (We are using it for Controllers for example.)
<T>this.inherit(methodMeta[index]), | ||
target, | ||
); | ||
return localMeta; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two methods (processInherited
and processLocal
) look too similar to me, it's hard to tell what are the important differences and what is the shared boiler-plate. Could you please try another round of extracting shared code to clean this up?
For example, the following block of code can be easily extracted into a shared helper method.
// processInherited
let methodMeta = baseMeta[method];
if (methodMeta == null) {
// Initialize the method metadata
methodMeta = new Array(
this.getNumberOfParameters(target, methodName),
).fill(undefined);
baseMeta[method] = methodMeta;
}
// processLocal
let methodMeta = localMeta[method];
if (methodMeta == null) {
// Initialize the method metadata
methodMeta = new Array(
this.getNumberOfParameters(target, methodName),
).fill(undefined);
localMeta[method] = methodMeta;
}
With my proposed helper:
// processInherited
const methodMeta = getOrCreateMeta(baseMeta, target, method);
// processLocal
const methodMeta = getOrCreateMeta(localMeta, target, method);
// getOrCreateMeta(meta, target, method)
let methodMeta = meta[method];
if (methodMeta == null) {
// Initialize the method metadata
methodMeta = new Array(
this.getNumberOfParameters(target, method || ''),
).fill(undefined);
meta[method] = methodMeta;
}
return methodMeta;
@@ -102,11 +102,10 @@ describe('model decorator', () => { | |||
|
|||
it('adds property metadata', () => { | |||
const meta = Reflector.getOwnMetadata( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use MetadataInspector
in these tests please? That way the tests are decoupled from the internal implementation details of *DecoratorFactory
.
7cd2e7e
to
d8a795b
Compare
5b19e21
to
5df313d
Compare
@bajtos @kjdelisle I believe that all of your comments have been addressed. PTAL. |
@loopback/metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!
I have few more comments, the one about the boolean arg ownOnly
is important to address as part of this initial pull request, the others can be deferred to follow-up pull requests if you like.
packages/metadata/README.md
Outdated
[reflect-metadata](https://github.com/rbuckton/reflect-metadata) | ||
* Decorator factories: A set of factories for class/method/property/parameter | ||
[decorators](https://www.typescriptlang.org/docs/handbook/decorators.html) to | ||
apply metadata to a given class and its members. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please beef up this README? I'd like to make this module useful to people outside LoopBack community too, so that we can gain more usage and more contributions. In order to get that, we need the README to be compelling enough for people who discover this package via search on npmjs.com.
See our README guidelines here: https://loopback.io/doc/en/contrib/README-guidelines.html. "Basic use" is the part I am missing most, e.g. how to use a decorator factory to create a decorator and then how to read the metadata stored by the decorator using the inspector.
I am ok with landing this pull request with the current insufficient README as long as the README is updated very soon afterwards, preferably before releasing this new package to npmjs.
packages/metadata/README.md
Outdated
@@ -0,0 +1,35 @@ | |||
# @loopback/metadata | |||
|
|||
This module contains utilities to manipulate TypeScript metadata. It includes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to include a short explanation of the common mistakes people make when using reflect-metadata and how this package is taking care of those tricky parts.
packages/metadata/src/inspector.ts
Outdated
static getClassMetadata<T>( | ||
key: string, | ||
target: Function, | ||
ownOnly?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not catching this earlier. Boolean arguments are an anti-pattern - how are the readers of the code invoking this method going to understand what true
or false
means?
const meta = getClassMetadata('mykey', myTarget, false);
Such API is also difficult to extend later, when we need to add more flags than just ownOnly
.
I am proposing to change all methods accepting a boolean ownOnly
argument to accept an "options" or "flags" object.
interface InspectOptions {
ownOnly?: boolean
}
static getClassMetadata<T>(
key: string,
target: Function,
options: InspectOptions = {})
packages/rest/src/router/metadata.ts
Outdated
spec = resolveControllerSpec(constructor, spec); | ||
Reflector.defineMetadata(API_SPEC_KEY, spec, constructor); | ||
spec = resolveControllerSpec(constructor); | ||
MetadataInspector.Reflector.defineMetadata( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we did not have to use low-level Reflector API here and leverage either class-level decorator factory or something like MetadataInspector.setClassMetadata
(a new method I just made up now).
Not a big deal, the patch can be landed with the current implementation too.
The feature implements common behavior for decorator functions for classes, methods, parameters, and properties. It takes inheritance into consideration and aggregate metadata for properties, methods, and parameters into one object by type per class. It also allows control for decorator inheritance by setting the `allowInheritance` flag or overriding `inherit()`.
Add a new module `@loopback/metadata` to contain utilities for: - Metadata reflection - Decorator factories to create decorators
076afa8
to
1f639b4
Compare
@bajtos Your comments have been addressed. PTAL. |
packages/metadata/src/inspector.ts
Outdated
* Options for inspection | ||
*/ | ||
export interface InspectionOptions { | ||
// Only inspect own metadata of a given target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to use TSDoc comment here.
/**
* Inspect own metadata of a given target only,
* ignore any metadata inherited from parent classes.
* By default, inspector returns both own and inherited metadata.
*/
ownMetadataOnly?: boolean;
1f639b4
to
18f3fe1
Compare
Description
The feature implements common behavior for decorator functions
for classes, methods, parameters, and properties. It takes
inheritance into consideration and aggregate metadata for properties,
methods, and parameters into one object by type per class.
@loopback/metadata
module to host utilities for:Checklist