Skip to content

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.

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.

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

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