[BUGFIX] Only freeze empty array/dict with weakmap #361
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When arrays and objects are frozen in JavaScript, it is impossible to attach meta-data (like Ember's own `meta`) to them without using a WeakMap. Ember does adopt the WeakMap strategy in browsers that support it, however there are still supported environments (IE9, IE10) where `Object.freeze` is supported but WeakMap is not. But not freezing these empty arrays and object if WeakMap is missing, legacy meta-data strategies are permitted on those instances. See: * emberjs/ember.js#14264 * emberjs/ember.js#14244
| let instance = new WeakMap(); | ||
| // use `Object`'s `.toString` directly to prevent us from detecting | ||
| // polyfills as native weakmaps | ||
| return Object.prototype.toString.call(instance) === '[object WeakMap]'; |
There was a problem hiding this comment.
Unfortunately, core-js monkey patches toString to even emulate ^. Given that, it is unclear to me if we can detect the real thing any better then how you have written it.
There was a problem hiding this comment.
Confirm, I did a ton of research around this before settling for it in ember itself. I just don't think there is a better way that isn't more brittle than this check.
|
|
|
Merging this. @rwjblue getting this into Ember release is going to be tricky I presume? We can sync up tomorrow and maybe I can help. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When arrays and objects are frozen in JavaScript, it is impossible to attach meta-data (like Ember's own
meta) to them without using a WeakMap. Ember does adopt the WeakMap strategy in browsers that support it, however there are still supported environments (IE9, IE10) whereObject.freezeis supported but WeakMap is not. But not freezing these empty arrays and object if WeakMap is missing, legacy meta-data strategies are permitted on those instances.The change here is untested, however I will add upstream tests in Ember for the relevant cases.
See:
The approach here is pretty naive. Some other more complex approaches I considered were:
env. This, however, would mean the empty evaluated and compiles positional and named arguments instances could not be eagerly created.HAS_NATIVE_WEAKMAPvalue from Glimmer'sglimmer-runtimepackage and use that instead of Ember's implementation. This would remove some small duplication, but it feels a bit weird to haveHAS_NATIVE_WEAKMAPas a glimmer export.Happy to make edits in one of those directions.