Use WeakMap for storing Glimmer metadata #119
Merged
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
Previously, tests were not running in strict mode, because we were emitting them as a JavaScript module (which implies strict mode) but not consuming them as such, so they defaulted to sloppy mode. In practice, this meant that several tests that verified compatibility with frozen objects were inappropriately passing, because mutation of frozen objects in sloppy mode is a no-op rather than an error. This commit changes the Rollup configuration for tests to use `iife` as the target format rather than `es`, which produces the appropriate `"use strict";` pragma.
Tracked properties rely on storing unique tags for each property on an object. We store these and other metadata in instances of a class called Meta, which are created lazily the first time a tracked property is requested for a property on an object. Previously, we used a symbol to store the object's Meta on itself. The symbol prevented inadvertent access or naming collisions. However, setting a property, even via a symbol, throws an error if the object in question is frozen via `Object.freeze`. It is a common pattern for libraries to use `Object.freeze` on objects that should be read-only, such as Apollo (#632). Simply reading a property from an object should not cause an exception. To mitigate this, this commit changes the implementation to use a WeakMap to map objects to their corresponding Meta instance. The new implementation is intended to be semantically identical to the previous implementation, but with compatibility with frozen objects.
rwjblue
approved these changes
Mar 11, 2018
This was referenced Mar 11, 2018
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.
TL;DR
WeakMapinstead of symbols to store tracked property metadata.Tracked properties rely on storing unique tags for each property on an object. We store these and other metadata in instances of a class called Meta, which are created lazily the first time a tracked property is requested for a property on an object.
Prior to this PR, we used a symbol to store the object's Meta on itself. The symbol prevented inadvertent access or naming collisions. However, when in strict mode, setting a property throws an error if the object in question is frozen via
Object.freeze. It is a common pattern for libraries to useObject.freezeon objects that should be read-only, such as Apollo (glimmerjs/glimmer-vm#632). Simply rendering a property from a frozen object in a template should not cause an exception to be thrown.To fix this, this commit changes the implementation to use a WeakMap to map objects to their corresponding Meta instance. The new implementation is intended to be semantically identical to the previous implementation, but with compatibility with frozen objects.
This PR contains another important change. We have dealt with issues with frozen objects in the past, and even wrote quite a few tests to verify compatibility with them.
Unfortunately, due to a misconfiguration I made to the Rollup builds, our test suite was not actually running in strict mode. This meant that all of the tests testing frozen objects were invalid, because in sloppy mode, mutating a frozen object does not throw an error.
This PR changes the Rollup configuration to use the
iifeformat rather thanes, which puts them into strict mode. After I made this change, previously passing tests began to fail. However, they are addressed by the move to WeakMaps.