[BUGFIX RELEASE] Fix toString to not add the property on the class #13333
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 calling `toString()` method on an Ember Object, it attaches the `toString` as a class property for the first time. This does not seem right since this should be on the prototype of the class. We could memoize the value on the class meta data. In v2.5.0 and above, when calling `.set()` on an Ember Object, you end up also attaching the `toString()` property on the class. This [PR](3a2486e) calls `toString()` on the object during setting any property. Therefore, when you walk through the object properties you end up getting an additional property on the object. cc: @krisselden @chadhietala
kratiahuja
changed the title
|
This looks good to me, CI just needs to be bounced once #13334 lands. Currently master is 🔴 . |
|
@rwjblue just to explain this, toString() mutates the shape of object and in the 2.5.0 release an assertion called object toString() in Ember.set() and while in production, asserts are stripped this failed tests that were asserting keys() because Ember.set() was adding a toString() to this. |
|
I have no clue why we would want to write |
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 calling
toString()method on an Ember Object, it attaches thetoStringas a class property for the first time. This does not seem right since this should be on the prototype of the class. We could memoize the value on the class meta data.In v2.5.0 and above, when calling
.set()on an Ember Object, you end up also attaching thetoString()property on the class. This PR callstoString()on the object during setting any property. Therefore, when you walk through the object properties you end up getting an additional property on the object.Thanks to Kris & Chad for guiding on this!
cc: @krisselden @chadhietala