Skip to content

Added reflection support #227

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

Merged
merged 6 commits into from
Oct 9, 2018
Merged

Conversation

realglebivanov
Copy link

@realglebivanov realglebivanov commented Feb 19, 2018

Hello! Currently i'm writing a library which uses properties custom reflection metadata.

The problem is that the metadata is lost due to prototype of prototype extension and I don't have a way to solve that problem except creating class decorator instead of property decorators.

I think there should be some possibility to clone that metadata in newly created class or so.
I will provide you a brief example.

@WithRender
@Component
export class TestComponent extends Vue {
    @Inject
    public service?: Service;
}

Metadata defined by @Inject decorator is lost. Possible solution is:

@Inject({service: Service})
@WithRender
@Component
export class TestComponent extends Vue {
    public service?: Service;
}

But I don't like it because idea is a property injection. Also, I'm thinking of lifecycle methods DI, but I'm not sure if it's useful.

Copy link
Member

@ktsn ktsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!
Can we revert all changes to files under dist/ directly since it should only updated during publishing process?

src/component.ts Outdated
@@ -1,4 +1,6 @@
import 'reflect-metadata'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of adding dependency that some users may not need as it bloats the lib size. Can we remove the dep and make this feature be optional?
I think only forwarding metadata when Reflect metadata is supported would work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's reasonable, I'll fix it.

@@ -23,13 +25,19 @@ export function componentFactory (
Component: VueClass<Vue>,
options: ComponentOptions<Vue> = {}
): VueClass<Vue> {
const reflectionMap: ReflectionMap = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need such temporary object and to process for it in different functions? Why don't we just directly assign the original metadata to component properties?

Copy link
Author

@realglebivanov realglebivanov Feb 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean ComponentOptions? If you mean that, it's a tricky way because i don't want to change options interface and expose internal work of Component decorator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't mean that.
I mean why we need to store metadata in temporary object rather than just forward them directly on the place.
https://github.com/vuejs/vue-class-component/pull/227/files#diff-7db1d7c8dc88ce00d1aa236fa0c128ccR40
https://github.com/vuejs/vue-class-component/pull/227/files#diff-7db1d7c8dc88ce00d1aa236fa0c128ccR118

Copy link
Author

@realglebivanov realglebivanov Feb 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because forwarding code will be duplicated. If you okay with that, I'll do it in the way you are asking. Also, we don't have a new class object until almost the end of function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, then we can extract the logic as a function. I believe we can remove reflectionMap without duplication.

something like:

function forwardMetadata(to: typeof Vue, from: typeof Vue, propertyKey: string): void {
  Reflect.getOwnMetadataKeys(from, propertyKey).forEach(metaKey => {
    const metadata = Reflect.getOwnMetadata(metaKey, from, propertyKey)
    Reflect.defineMetadata(metaKey, metadata, to, propertyKey)
  })
}

Then we just call it inline:

  Object.getOwnPropertyNames(Original).forEach(key => {
     // `prototype` should not be overwritten
     if (key === 'prototype') {
       return
     }
 	 
-    reflectionMap.static[key] = Reflect.getOwnMetadataKeys(Original, key);
+    forwardMetadata(Extended, Original, key)

     // Some browsers does not allow reconfigure built-in properties
     const extendedDescriptor = Object.getOwnPropertyDescriptor(Extended, key)
     if (extendedDescriptor && !extendedDescriptor.configurable) {
  forwardStaticMembersAndCollectReflection(Extended, Component, Super, reflectionMap)
-  copyReflectionMetadata(Component, Extended, reflectionMap)
+  Object.getOwnPropertyNames(proto).forEach(key => {
+    forwardMetadata(Extended.prototype, proto, key)
+  })

src/component.ts Outdated
@@ -69,7 +77,8 @@ export function componentFactory (
: Vue
const Extended = Super.extend(options)

forwardStaticMembers(Extended, Component, Super)
forwardStaticMembersAndCollectReflection(Extended, Component, Super, reflectionMap)
copyReflectionMetadata(Component, Extended, reflectionMap)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this if we directly forward metadata and remove reflectionMap.

@ktsn
Copy link
Member

ktsn commented Feb 19, 2018

Sorry, I overlooked somethings. Can you fix the code format to follow the existing one (indent space 2, and no-semicolons)?

@realglebivanov
Copy link
Author

Thank you for your quick responses. Of course i can fix codestyle.

@realglebivanov
Copy link
Author

Done.

Review this PR, please.

@304NotModified
Copy link
Contributor

Done.

bump @ktsn

@ktsn ktsn merged commit a362138 into vuejs:master Oct 9, 2018
ktsn added a commit that referenced this pull request Oct 9, 2018
@ktsn
Copy link
Member

ktsn commented Oct 9, 2018

Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants