Introduce strongly-typed metadata key #1240

raymondfeng · 2018-04-09T22:40:02Z

This PR introduces a strong-typed MetadataKey for metadata access via reflection. It's inspired by #1169.

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

shimks

Had a quick overview, and most of it looked good. A couple of points though:

MetadataAccessorneeds it own test, similar to how BindingKey had its own test in that other PR
Couple packages had been missed out when the others' keys were converted to use MetadataAccessor
- authentication
- repository-json-schema (feel free to separate out the key to its own file for this one (didn't do this when I created the package :p))
- there might be others, so I'd check other packages just to be sure
did you check for potential changes in the docs that would use this feature?

shimks · 2018-04-10T14:28:15Z

packages/metadata/test/unit/inspector.test.ts

+  const TEST_META = MetadataAccessor.create<object, ClassDecorator>('test');
+
  it('inspects metadata of a base class', () => {
    const meta = MetadataInspector.getClassMetadata('test', BaseController);


Why isn't test being replaced with TEST_META here?

I wanted to test string keys too.

shimks · 2018-04-10T14:34:03Z

packages/metadata/README.md

+const myClassDecorator = ClassDecoratorFactory.createDecorator(CLASS_KEY);
+
+// Inspect a class with the key
+const myClassMeta = MetadataInspector.getClassMetaData(CLASS_KEY, MyController);


Can we include a comment here insinuating that myClassMeta will have its type set to MyClassMetadata?

shimks

Also, make sure that the new file is mentioned in docs.json

raymondfeng · 2018-04-11T15:55:14Z

@shimks PTAL

virkt25 · 2018-04-11T17:38:01Z

packages/repository-json-schema/src/build-schema.ts

+import {MetadataInspector, MetadataAccessor} from '@loopback/context';

-export const JSON_SCHEMA_KEY = 'loopback:json-schema';
+export const JSON_SCHEMA_KEY = MetadataAccessor.create<JsonDefinition>(


Why is there no decorator type passed here in the <>?

If missing, any decorator type is fine. There are valid cases that the same string is used for both class and member decorators.

Didn't realize it still works with just key strings. :)

virkt25 · 2018-04-11T17:38:36Z

packages/metadata/src/types.ts

+export class MetadataAccessor<T, D extends DecoratorType = DecoratorType> {
+  private constructor(public readonly key: string) {}
+
+  toString() {


Isn't it cleaner and easier to access the .key property directly?

Please note toString() works for both MetadataAccessor and string so that we don't to test if the key is a string or not.

virkt25 · 2018-04-11T17:40:31Z

packages/metadata/src/inspector.ts

      options && options.ownMetadataOnly
-        ? Reflector.getOwnMetadata(key, target)
-        : Reflector.getMetadata(key, target);
+        ? Reflector.getOwnMetadata(key.toString(), target)


Not a blocker but using .toString() is messy imo. Wouldn't it be better to do key.key or key.name with us storing the key as the name property?

See my explanation above.

shimks

LGTM, but please wait on merging this until @bajtos's review since the PR itself had been inspired by the PR he authored.

shimks · 2018-04-11T18:09:47Z

packages/metadata/test/unit/metadata-accessor.test.ts

+    ).to.equal('bar');
+  });
+
+  it('can be used to create decorator', () => {


Might be nitpicky, but I think this test should live in a separate integration folder

virkt25 · 2018-04-11T19:28:24Z

packages/authentication/src/keys.ts

-import {BindingKey} from '@loopback/context';
 import {AuthenticateFn, UserProfile} from './providers/authentication.provider';
 import {AuthenticationMetadata} from './decorators/authenticate.decorator';
+import {BindingKey, MetadataAccessor} from '@loopback/context';


Shouldn't MetadataAccessor be imported from @loopback/metadata? Actually I'm not even sure how this is working since I didn't see a re-export of MetadataAccessor from @loopback/context and this class is defined in @loopback/metadata.

See https://github.com/strongloop/loopback-next/blob/master/packages/context/src/index.ts#L6

virkt25 · 2018-04-11T19:59:51Z

packages/metadata/src/types.ts

+ * @typeparam T Type of the metadata value
+ * @typeparam D Type of the decorator
+ */
+export class MetadataAccessor<T, D extends DecoratorType = DecoratorType> {


The class shouldn't be in a types.ts file. Should have it's own file.

If we move MetadataAccessor to its own file, it will create a circular deps:

MetadataAccessor --> DecoratorType (metadata-accessor.ts)

MetadataKey --> MetadataAccessor (types.ts)

BTW, a class plays dual roles of typing and implementing.

I know a class serves as a type but just thought types.ts for was types / interfaces not classes.

Didn't see the circular dependency. Good enough reason to keep it here.

bajtos

I quickly skimmed through the patch and don't see any major issues.

Is there any additional documentation (either as API docs or in /docs/site) that should be updated to reflect these changes?

raymondfeng · 2018-04-12T15:40:07Z

Is there any additional documentation (either as API docs or in /docs/site) that should be updated to reflect these changes?

I have updated the README, which is the docs we pull into loopback.io for metadata.

raymondfeng requested a review from bajtos as a code owner April 9, 2018 22:40

raymondfeng added needs-priority and removed needs-priority labels Apr 9, 2018

raymondfeng changed the title ~~Metadata key~~ Introduce strongly-typed metadata key Apr 10, 2018

shimks suggested changes Apr 10, 2018

View reviewed changes

shimks reviewed Apr 10, 2018

View reviewed changes

raymondfeng force-pushed the metadata-key branch 2 times, most recently from 4549518 to 8433547 Compare April 11, 2018 15:55

raymondfeng force-pushed the metadata-key branch from 8433547 to 2dd8b81 Compare April 11, 2018 16:28

virkt25 reviewed Apr 11, 2018

View reviewed changes

shimks approved these changes Apr 11, 2018

View reviewed changes

feat(metadata): add strongly-typed metadata accessors

1bcb9b4

raymondfeng force-pushed the metadata-key branch from 2dd8b81 to 1bcb9b4 Compare April 11, 2018 19:15

virkt25 reviewed Apr 11, 2018

View reviewed changes

virkt25 approved these changes Apr 12, 2018

View reviewed changes

bajtos approved these changes Apr 12, 2018

View reviewed changes

raymondfeng merged commit 45f9f80 into master Apr 12, 2018

raymondfeng removed the review label Apr 12, 2018

raymondfeng deleted the metadata-key branch April 12, 2018 15:40

dhmlau mentioned this pull request Apr 30, 2018

Monthly Milestone - April 2018 #1044

Closed

23 tasks

Introduce strongly-typed metadata key #1240

Introduce strongly-typed metadata key #1240

Uh oh!

Conversation

raymondfeng commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

shimks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks left a comment

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Apr 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

raymondfeng commented Apr 9, 2018 •

edited

Loading