-
Notifications
You must be signed in to change notification settings - Fork 433
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
Conversation
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.
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' |
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'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.
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.
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 = { |
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 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?
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.
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.
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.
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
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.
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.
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, 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) |
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 don't need this if we directly forward metadata and remove reflectionMap
.
Sorry, I overlooked somethings. Can you fix the code format to follow the existing one (indent space 2, and no-semicolons)? |
Thank you for your quick responses. Of course i can fix codestyle. |
Done. Review this PR, please. |
bump @ktsn |
Thank you for your contribution. |
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.
Metadata defined by @Inject decorator is lost. Possible solution is:
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.