[BUG]: Consume array access to autotrack hasMany #7330
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
|
Performance Report for a92c2aa Relationship Analysis |
|
Asset Size Report for a92c2aa IE11 Builds The size of the library EmberData has increased by 93.0 B (29.0 B compressed) which exceeds the failure threshold of 15 bytes.WarningsThe uncompressed size of the package @ember-data/model has increased by 93.0 B. Changeset Full Asset Analysis (IE11)Modern Builds The size of the library EmberData has increased by 93.0 B (29.0 B compressed) which exceeds the failure threshold of 15 bytes.WarningsThe uncompressed size of the package @ember-data/model has increased by 93.0 B. Changeset Full Asset Analysis (Modern)Modern Builds (No Rollup) EmberData increased by 93.0 B uncompressed but decreased by 0.0 B compressedWarningsThe uncompressed size of the package @ember-data/model has increased by 93.0 B. Changeset Full Asset Analysis (Modern) |
snewcomer
commented
Sep 20, 2020
| @@ -166,6 +166,13 @@ export default EmberObject.extend(MutableArray, DeprecatedEvented, { | |||
| return false; | |||
| }, | |||
|
|
|||
| toArray() { | |||
| // By using `get()`, the tracking system knows to pay attention to changes that occur. | |||
| get(this, '[]'); | |||
There was a problem hiding this comment.
@pzuraq 👋 I'm noticing we have to consume every time it is accessed. Caching on this once when accessed and preventing further consuming doesn't pass the tests I added here.
I'm thinking this might be expected since sortBy (through toArray && map) returns a new array each time. What do you think?
There was a problem hiding this comment.
Discussed this in the data meeting but wanted to note it down here, the issue is that length also needs to be instrumented here, along with objectAt. This because objectAt is not used when the length is 0, but length is read to check that.
rwjblue
approved these changes
Sep 24, 2020
snewcomer
force-pushed
the
sn/has-many-consume
branch
from
dd9a47a to
6aa7887
Compare
igorT
reviewed
Sep 24, 2020
| // By using `get()`, the tracking system knows to pay attention to changes that occur. | ||
| get(this, '[]'); | ||
|
|
||
| if (typeof this._length === 'number') { |
There was a problem hiding this comment.
Why do we need this check? Should we set the length in init rather than have to do this every time you access length?
There was a problem hiding this comment.
Either works! I did that but then thought I would keep all the logic in one place. I can do it in init if you would like!
snewcomer
force-pushed
the
sn/has-many-consume
branch
7 times, most recently
from
f2cf8bf to
7880de8
Compare
snewcomer
force-pushed
the
sn/has-many-consume
branch
from
7880de8 to
a92c2aa
Compare
|
Hi everyone, seems like this is ready to go, no? Really sorry for the pressure, but this bug is really making it hard to debug tracking bugs on my update to autotracking. Thanks again! :) |
rwjblue
approved these changes
Oct 7, 2020
There was a problem hiding this comment.
Can you drop the second commit here, and we can land this (for release in [email protected])?
snewcomer
force-pushed
the
sn/has-many-consume
branch
from
cb4ba29 to
a92c2aa
Compare
This comment has been minimized.
This comment has been minimized.
rwjblue
approved these changes
Oct 9, 2020
snewcomer
pushed a commit
that referenced
this pull request
Oct 9, 2020
[BUG]: Consume array access to autotrack hasMany
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.
close #7311 emberjs/ember.js#19101 #7328 emberjs/ember.js#19139
ref emberjs/ember.js#19105 #7281
Another related autotracking fix for
errorsFor#7273In this PR
Has Many Arrays implement the Mutable Array mixin. We need to instrument
lengthandobjectAtin order to ensure autotracking works.To do
Follow up: convert to class based && ts file