attributeBindings is now frozen in debug builds on v2.11, slice it before pushing #59
Conversation
|
I did try upgrading ember-test-selectors to ember-cli and ember v2.11: https://github.com/simplabs/ember-test-selectors/compare/master...bgentry:ember-cli-2.11-upgrade?expand=1 However, I did not encounter any failures like I was receiving in my app. I guess the failure case isn't being exercised by any existing tests. In my app, the error occurs when trying to push to a non-empty Feel free to PR that upgrade branch if you'd like to use it as-is. |
As noted in the Ember v2.11 release notes, concatenated properties such as attributeBindings are frozen in debug builds. This means we cannot push directly onto the attributeBindings array without first copying it via slice. Without this change, users may see errors like this when the `attributeBindings.push()` call is made: Uncaught TypeError: Can't add property 2, object is not extensible References: emberjs/ember.js#14389 emberjs/ember.js#14601
bgentry
force-pushed
the
fix-frozen-attribute-bindings-array
branch
from
951872c
to
001b11e
Compare
|
Hm, somehow Travis doesn't seem to be running tests with 2.9, 2.10 and 2.11… We certainly should have a test case in here that fails for 2.11 without these changes. |
|
@rwjblue can I get your knowledge on this? freezing the |
|
attributeBindings, classNameBindings, classNames, etc are all mutable arrays that are stored on the components prototype. This means that mutating for one instance changes all instances (likely just changing for all future instances). To protect you (the user) from this, Ember used to slice these arrays for every component instance speculatively (when the majority of the time it is never mutated). So in Ember 2.11 these arrays are now frozen in dev builds so that mutating causes an error. This allows us to remove the speculative slicing (reducing wasted Array allocations and related GC pressure). It is still valid and supported to want to customize these arrays on a per instance basis (as long as it is done from within the constructor/init), but you have to slice (to create a new array instance) before mutating (to avoid the "mutating a frozen object" error messages). |
|
@rwjblue okay, so what I'm understanding is: what we are doing is okay, as long as we slice and don't try to mutate. do we have to modify the field before or after the |
|
We only look for the value after the constructor is completed, so it doesn't really matter about setting attributeBindings before or after calling super. In general, you should always call super before touching this (same rule as ES6 classes) which is what I would fall back on in this case. |
|
@bgentry I've got a test case now that verifies that your PR succesfully fixes this issue. Thanks a lot! |
|
Awesome, thanks all 🙌 |
As noted in the Ember v2.11 release notes, concatenated properties such as attributeBindings are frozen in debug builds. This means we cannot push directly onto the attributeBindings array without first copying it via slice.
Without this change, users may see errors like this when the
attributeBindings.push()call is made:References:
classNamesandclassNameBindingsspeculatively. emberjs/ember.js#14389