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

Consider WeakMap implementation of Reflect metadata. #34

Closed
jods4 opened this issue Sep 12, 2016 · 4 comments
Closed

Consider WeakMap implementation of Reflect metadata. #34

jods4 opened this issue Sep 12, 2016 · 4 comments

Comments

@jods4
Copy link
Contributor

jods4 commented Sep 12, 2016

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

@EisenbergEffect
Copy link
Contributor

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.

@jods4
Copy link
Contributor Author

jods4 commented Sep 12, 2016

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.

@Alexander-Taran
Copy link

Stale since 2016

@EisenbergEffect
Copy link
Contributor

We'll probably remove Reflect.metadata in vNext.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants