[BUGFIX] Setting ArrayProxy#content in willDestroy resets length. #16784
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
krisselden
reviewed
Jun 28, 2018
| @@ -230,6 +230,9 @@ export default class ArrayProxy extends EmberObject { | |||
|
|
|||
| this.arrayContentDidChange(0, oldLength, newLength); | |||
| this._addArrangedContentArrayObsever(); | |||
| } else if (key === 'content') { | |||
There was a problem hiding this comment.
at this point we should extract into an invalidate method
There was a problem hiding this comment.
Updated to use a shared _invalidate method.
Prior to this change, `ArrayProxy.prototype.length` was marked as dirty (and therefore recomputed) when `content` was set due to `arrangedContent` being an alias of `content`, and `ArrayProxy` implementing `PROPERTY_DID_CHANGE` to trap `arrangedContent` changes. Unfortunately, `notifyPropertyChange` avoids notifying _some_ things when the object is destroying. This results in the preexisting `PROPERTY_DID_CHANGE` trap for `arrangedContent` is no longer called, and therefore `length` is never marked as dirty. This fixes the condition, by implementing a `content` trap in `PROPERTY_DID_CHANGE` that marks length as dirty. The next time that `length` is accessed it will do the normal work to recalculate.
rwjblue
force-pushed
the
moar-array-proxy-tweaks
branch
from
c081c09 to
7189bf0
Compare
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.
Prior to this change,
ArrayProxy.prototype.lengthwas marked as dirty (and therefore recomputed) whencontentwas set due toarrangedContentbeing an alias ofcontent, andArrayProxyimplementingPROPERTY_DID_CHANGEto traparrangedContentchanges.Unfortunately,
notifyPropertyChangeavoids notifying some things when the object is destroying. This results in the preexistingPROPERTY_DID_CHANGEtrap forarrangedContentis no longer called, and thereforelengthis never marked as dirty.This fixes the condition, by implementing a
contenttrap inPROPERTY_DID_CHANGEthat marks length as dirty. The next time thatlengthis accessed it will do the normal work to recalculate./cc @krisselden @runspired