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

[BUGFIX beta] Ensure params and hash are frozen in debug builds. #14244

Merged
merged 2 commits into from Sep 9, 2016

Conversation

Projects
None yet
3 participants
@rwjblue
Member

rwjblue commented Sep 9, 2016

This ensures that params and hash are both ran through Object.freeze in debug builds, but tries to leave the codepaths as simple as possible so that when the runInDebug's are removed the production code can be optimized the same way as prior to this change.

Closes #14189.

Robert Jackson
Add `debugFreeze` debug helper.
This is stripped in production builds, but is useful to help ensure that
a given object is not mutated.
@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Sep 9, 2016

Member

This is nice! Do you have any idea if there's a performance penalty?

Member

mmun commented Sep 9, 2016

This is nice! Do you have any idea if there's a performance penalty?

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Sep 9, 2016

Member

Yes, adding Object.freeze will definitely have some cost. There should be basically no cost when this is in production builds, since the entire runInDebug statement is removed. I'm hoping the added "correctness" and the fact that prod builds will be the same will make up from any performance impact.

I'm also open to reverting if it turns out that this makes even debug builds too slow, but I generally think that will be unlikely...

Member

rwjblue commented Sep 9, 2016

Yes, adding Object.freeze will definitely have some cost. There should be basically no cost when this is in production builds, since the entire runInDebug statement is removed. I'm hoping the added "correctness" and the fact that prod builds will be the same will make up from any performance impact.

I'm also open to reverting if it turns out that this makes even debug builds too slow, but I generally think that will be unlikely...

Robert Jackson
[BUGFIX beta] Ensure params and hash are frozen in debug builds.
This ensures that `params` and `hash` are both ran through
`Object.freeze` in debug builds, but tries to leave the codepaths as
simple as possible so that when the `runInDebug`'s are removed the
production code can be optimized the same way as prior to this change.

@rwjblue rwjblue merged commit be4fb63 into master Sep 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rwjblue rwjblue deleted the deep-freeze branch Sep 9, 2016

@mixonic mixonic added the Bug label Nov 29, 2016

mixonic added a commit to mixonic/glimmer that referenced this pull request Dec 17, 2016

[BUGFIX] Only freeze empty array/dict with weakmap
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

chancancode added a commit to glimmerjs/glimmer-vm that referenced this pull request Jan 10, 2017

[BUGFIX] Only freeze empty array/dict with weakmap
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

(cherry picked from commit 6954ade)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment