Reflect.getOwnMetadata gets metadata for parents in Typescript

[doktordirk]

edit:
Current Reflect polyfill doesn work for child classes in typescript since Object.hasOwnProperty(key) returns the parent if applicable

---

old message:
@cmichaelgraham following up on aurelia/metadata#33

class x {
  meta;
}
class y extends x{}

y.hasOwnProperty('meta') should be false.

this works as specified in es6 but not for typescript where current version yields true.

https://github.com/aurelia/polyfills/blob/master/src/reflect.js#L21

let metadataContainer = target.hasOwnProperty(metadataContainerKey) ? target[metadataContainerKey] : (target[metadataContainerKey] = {});
let targetContainer = metadataContainer[targetKey] || (metadataContainer[targetKey] = {}); 

if target.hasOwnProperty gives wrongly true for child classes, the parent target[metadataContainerKey] is used for metadataContainer. and since targetKey is undefined and the parent has a metadataContainer['undefined'] the parents targetContainer is used instead of a new one.

sorry if i'm not clear enough, but finding the issue wasn't particular easy. m mushy in the head.
point begin: hasOwnProperty isn't behaving right for TS

[EisenbergEffect]

Would you be interested to submit a PR to fix this? 😄

[doktordirk]

hell no 😄
that'll be far over my head i think. (though i'll have a peak nonetheless)

[doktordirk]

@EisenbergEffect i assume microsoft/TypeScript#1601 is the issue. something different in the extends impelemtation of TS

[doktordirk]

hasOwnProperty on TS playground

http://www.typescriptlang.org/play/#src=class%20A%20%7B%0Afoo%3Astring%20%3D%20'bar'%0A%7D%0Aclass%20B%20extends%20A%20%7B%0Abar%3Astring%20%3D%20'foo'%0A%7D%0A%0Aconsole.log((new%20A()).hasOwnProperty('foo'))%0Aconsole.log((new%20B()).hasOwnProperty('foo'))%0Aconsole.log((new%20A()).hasOwnProperty('bar'))%0Aconsole.log((new%20B()).hasOwnProperty('bar'))%0A

[doktordirk]

as it seems difficult to distinguish parent and child properties in typescript, the only/best solution would be to use/follow https://github.com/rbuckton/ReflectDecorators/blob/master/Reflect.js . they keep a weapmap based on the target for all metadatas. i'm not sure atm if there is another solution than a seperate metadata list

[doktordirk]

@EisenbergEffect i'm still sortof battling with current Reflect poyfill. i tired to look up the specifications, but didn't find it for the used methods. only found https://tc39.github.io/ecma262/#sec-reflection which is different from whats used here!?

[EisenbergEffect]

The metadata methods used here are part of the Metadata proposal for ES Next.

[doktordirk]

@EisenbergEffect
that seem to be quite far from getting implemented. it's not even yet proposed and the original specs have been deleted!

see some discussion here zloirock/core-js#152

[EisenbergEffect]

I submitted an issue to @rbuckton 's repo. We'll see what they say. We do need to associate metadata with classes, so it's going to stay in some form. The only question is whether we need to update the implementation in some way.

[doktordirk]

since it's not working for typescript as it is, something has to be done. i see three options:

• tell typescript user to use there own polyfill (though i didn't manage that yet)
• use the implementation from eg https://github.com/gdi2290/es7-reflect-metadata
• make a hacky version by just adding/using target.name to/for the target key. (as opposed to leaving the key at undefinedas per Reflect spez for class metadata, i think)

[EisenbergEffect]

Have you tried the name hack...when the code is minified?

[doktordirk]

unminified worked for plugins when calling (aurelia-)metadata.getOrCreateOwn(Metadata.key, Metadata, target**, target.name**);

had a quick try early on with getOrCreateOwn(metadataKey: string, Type: Function, target: Function, targetKey=target.name: string): Object; which cause errors for TS.

didn' try yet Reflect.defineMetadata = function(metadataKey, metadataValue, target, targetKey=target.name) or alike nor minified.

but i'll play around a bit

ps: minification shouldn't matter with this approach anyways

[doktordirk]

@EisenbergEffect @cmichaelgraham
while basically working just fine, it unfortunately creates new issues for Typescript with metadata.get

get(metadataKey: string, target: Function, targetKey?: string): Object {
    if (!target) { return undefined; }
    let result = metadata.getOwn(metadataKey, target, targetKey);
    return result === undefined ? metadata.get(metadataKey, Object.getPrototypeOf(target), targetKey) : result;
  },

that worked for TS before, since metadata.getOwn allways did get from the parent anyways.
if that's fixed for TS, then Object.getPrototypeOf(target) should be used to get the parents metadata if applicable, but which, surprise surprise, doesn't work with TS's extends. (so, coming back again to aforementioned Microsoft/TypeScript 1601)

that starts to beg the question, whether it wouldn't be better to force TS to use a standard compliant extends generally.

[doktordirk]

i found another possible solution: let typescript target es6 instead of es5. works with current edge/chrome/firefox. obviously older browser will get problems. dunno if chaining babel for 6to5 would works

[EisenbergEffect]

@doktordirk Do you have any other thoughts on this?

[doktordirk]

@EisenbergEffect since it'll be solved if the typescript to es6 transpiler is used, it'll be a no issue at some stage. meanwhile, i could add a hint with a workaround to the readme or somewhere else you point me to

[doktordirk]

ps: my commit message in our plugins reads like:

Typescript's 'extends' is on purpose not according to specifications. Therefore, Reflects usage with targettKey===undefinded of hasOwnProperty(targettKey) returns true for a parent property 'undefined' (The derived class doesn't have that at this point, thus false should have been returned).

A simple fix for us is not to use the key 'undefined' (as per specification) in this case, but use a key on basis of the class name. Since they are different, the faulty 'extends' doesn't propose an issue

so, instead of:

import {metadata} from 'aurelia-metadata';

export class OrmMetadata {
  static forTarget(target) {
    return metadata.getOrCreateOwn(Metadata.key, Metadata, target);
  }
}

use

import {metadata} from 'aurelia-metadata';

export class OrmMetadata {
  static forTarget(target) {
    return metadata.getOrCreateOwn(Metadata.key, Metadata, target, target.name);
  }
}

[EisenbergEffect]

Hmm. A note would be great...I'm just not sure where it should go. Do you have any thoughts on that. Where would you expect to find it as a community member?

[doktordirk]

I guess metadata be best. not that many will use it themself. i.ll try do PR something. on a general note. Maybe typescript tests should be included somehow

[doktordirk]

@EisenbergEffect
added it to the docblock, so it appears in the HUBs api docs. I think that would be the best place.
aurelia/metadata#38