Reflect.metadata polyfill is incorrect

[jods4]

Aurelia has it's own polyfills for metadata related features but Reflect.defineMetadata is buggy and creates problems.

The issue is that it uses ClassConstructor.__metadata__ as a backing storage without checking if it is its own field. This in turn means that a base class and all its sub-classes will share the same metadata store (at least with both TypeScript and Babel).

Here's what happen in details:

@autoinject
class B { }
class C extends B { }

When the code above is transpiled to ES5, both TS and Babel will create a function B and because of the attribute it will end up adding metadata to B through Reflect.defineMetadata.

With you implementation, this will get polyfilled as an object "dictionary" stored in B.__metadata__. So far so good.

Then when C extends B the static members of B are copied to C. TS and Babel differ here. TS simply copies every own key of B to C. Including __metadata__. Babel sets the prototype of C to B. Both have almost the same effect: reading C.__metadata__ will return B metadata storage.

And now those classes share the same metadata storage. For me the bug was exposed by this scenario: we have a base class for some view models. Aurelia caches view strategy into metadata. So depending on the order we browse our views, we ended up with a ViewModel incorrectly using another viewmodel's view. This is very bad!

I fixed the issues (temporarily) by providing my own polyfill of Reflect.defineMetadata. Specifically I changed this line:
https://github.com/aurelia/metadata/blob/master/src/metadata.js#L39
to:

var metaContainer = target.hasOwnProperty("__metadata__") ? target["__metadata__"] : (target["__metadata__"] = {});

The important difference here is that I only read the __metadata__ if it is the class' own property, not something coming from the prototype. Note that I believe this fixes the issue when using Babel only. Because Babel uses prototypes but TS simply copies the properties over, which means the fix is not going to work with TS.

For reference, rbuckton has implemented his polyfill using a WeakMap, which guarantees a distinct storage for each class, see:
https://github.com/rbuckton/ReflectDecorators/blob/master/Reflect.js#L23

[EisenbergEffect]

Please submit a PR for your fix. Thanks!

[jods4]

OK.
But you are aware that my fix doesn't work with Typescript? It's ok for me but it might not be good for everyone.

Given how TS copies the static members, a proper fix is harder without WeakMap support. 😒

[EisenbergEffect]

Yes, we can just update our docs and inform TypeScript users that they should load the Reflect polyfill.

[jods4]

OK I will.

Looking at the Core-js WeakMap polyfill, I think that it is buggy as well (no time to test right now). So with TS, using the Reflect.metadata polyfill is going to be OK for people targetting IE11 but not IE9 or IE10.

I think for those the only proper fix would be for TS to change its codegen. <_<

[EisenbergEffect]

You might open an issue with TS and explain the consequence of their generation strategy.

[jods4]

I submitted a PR.

I just realized that an easy work-around for TS users is to define the utility function __extends yourself (TS supports that).

I am going to open a bug at TS anyway, it would be best to fix it for everyone "by default".