Reflect.getOwnMetadata gets metadata for parents in Typescript

Question

Reflect.getOwnMetadata gets metadata for parents in Typescript

Closed this issue 8 years ago · 20 comments

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

Answer 1 · 2016-04-29T16:34:01.000Z

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

Answer 2 · 2016-04-29T16:40:46.000Z

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

Answer 3 · 2016-04-30T08:35:28.000Z

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

Answer 4 · 2016-04-30T12:12:36.000Z

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

Answer 5 · 2016-05-01T11:35:02.000Z

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

Answer 6 · 2016-05-02T12:30:41.000Z

@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!?

Answer 7 · 2016-05-02T13:32:34.000Z

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

Answer 8 · 2016-05-02T15:09:59.000Z

@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

Answer 9 · 2016-05-02T15:15:26.000Z

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.

Answer 10 · 2016-05-02T15:23:13.000Z

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)

Answer 11 · 2016-05-02T15:38:45.000Z

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

Answer 12 · 2016-05-02T15:57:56.000Z

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

Answer 13 · 2016-05-03T08:36:57.000Z

@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.

Answer 14 · 2016-05-03T10:17:48.000Z

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

Answer 15 · 2016-07-06T14:03:23.000Z

@doktordirk Do you have any other thoughts on this?

Answer 16 · 2016-07-06T16:32:24.000Z

@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

Answer 17 · 2016-07-06T16:36:56.000Z

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);
  }
}

Answer 18 · 2016-07-06T18:24:07.000Z

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?

Answer 19 · 2016-07-06T20:27:39.000Z

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

Answer 20 · 2016-07-07T07:03:11.000Z

@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