Consider WeakMap implementation of Reflect metadata.
Closed this issue · 4 comments
I was trying to get rid of Babel, since the nightly TS compiler now has ES5 emit for everything we use (including async). And I got bitten by the metadata vs inheritance bug again.
For reference, this was discussed several times already, by Aurelia and TS: #18, aurelia/metadata#22, microsoft/TypeScript#4266, microsoft/TypeScript#1601
The remaining problem today is that when emitting ES5 code, TS copies all members of parent class to descendants. Given the current Reflect code, which uses a field on the class, this means all inherited classes share the same metadata dictionary as their parent.
Suggestion: why not implement the Reflect metadata apis on top of a WeakMap? This is better because it doesn't modify the external object, and it fixes this bug where a real WeakMap is available (IE11).
For older browsers (IE9, 10) a polyfill is required and the problem will still be there. There is no perfect solution for those anyway, as not everything can be supported properly (see discussions in linked issues).
Yes. Ultimately, using a WeakMap is what we want. I think we had concerns about the Polyfills and potential memory leaks related to those. The other concern was performance.
Would you be at all interested in switching this over to use the WeakMap as a PR? With that done, we might be able to run some benchmarks and tests to see how that works out.
Did you have something specific in mind concerning polyfills? AFAIK they all do what your Reflect metadata implementation does: store the values directly on the target object inside a property with a unique name. So the outcome should be the same as today, I guess.
Hopefully native WeakMap performs better.
To be honest, our build contains a custom bundle of corejs polyfills (using core-js-builder), and I was planning to add Reflect.metadata to those, so that it would bypass Aurelia's (core-js polyfill is based upon WeakMap).
👉 This might also be a way for you to benchmark and test. No need to modify Aurelia, just include a Reflect polyfill before Aurelia starts.
Which leads me to: can we remove the aurelia-polyfills dependency? :) It's hardcoded inside aurelia-bootstrapper.
Stale since 2016
We'll probably remove Reflect.metadata in vNext.