Skip to content
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

fix(metadata): targetKey in Reflect.defineMetadata is optional #33

Merged
merged 1 commit into from
May 9, 2016

Conversation

doktordirk
Copy link
Contributor

as currently it isn't marked optional for typescript "undefined" is used for all if no key is given. That's not working.

alternatively setting a default as targetKey:string = target.name would work

@doktordirk
Copy link
Contributor Author

@cmichaelgraham any comments for me? you're the ts guy right

@EisenbergEffect
Copy link
Contributor

@doktordirk This looks right to me. Make sure to run the build and doc task locally just to make sure that the d.ts and docs generator process everything correctly. You don't need to commit those generated files though. I'll do that at release.

@doktordirk
Copy link
Contributor Author

i'll do more tests and come back then

@cmichaelgraham
Copy link
Contributor

looks good to me too 👍

@doktordirk
Copy link
Contributor Author

please wait till i've tested more. while the pr is obviously right, the problems is that the types on Reflect aren't used, as only exported functions enter the d.ts file. hence, it seems it doesn't fix the problem. i'm on it though

@doktordirk doktordirk closed this Apr 29, 2016
@EisenbergEffect
Copy link
Contributor

Ok, that's a different issues altogether. @cmichaelgraham can you look into this? There may be a bug in the d.ts generator.

@cmichaelgraham
Copy link
Contributor

Should be able to take a look at this tonight...

@doktordirk
Copy link
Contributor Author

seems though my issue comes from the reflect polyfill. targetKey===undefined leads to different results in es6 and ts

@doktordirk
Copy link
Contributor Author

since hasOwnProperty of Typescript doesn't work as specifies since 'extends' of Typescript 1.5 doesn't work as specified, on nice solution would be to default targetKey=target.name, since parent and child classes have different names, the problem with interitance would be fixed.

as i'd say positive side effect, since you don't use that optional paremeter yourself, (and maybe the Reflect implementation is wrong?), you register under the key 'undefined'. defaulting targetKey to target.name would make look the objects much nicer :).

that seems to work fine (by alrge). didn't have any issues with es6, with typescript (again!), however, @autoinject breaks (while inject works fine). i'll have a quick look why's that

@doktordirk
Copy link
Contributor Author

too many problems with above version.

i for my part, just use add 'target.name' as targetKey to Metadata.getOrCreateOwn() whenever it comes up for derived classes.
so, the fix for that is simple. whether this is a general issue which needs attention, i don't know

@EisenbergEffect EisenbergEffect merged commit fa861f7 into aurelia:master May 9, 2016
@EisenbergEffect
Copy link
Contributor

d.ts updates were correct so I got this merged.

@doktordirk doktordirk deleted the patch-1 branch May 16, 2016 13:26
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