-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[FEAT] Adds Array Tracking #17751
[FEAT] Adds Array Tracking #17751
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 💯! There's one bit of the implementation that should be lightly tweaked from the TS perspective, and I've left a comment to that effect.
Two questions this raises for me (that are born of my ignorance, with no implied commentary on the PR):
- What if anything are the performance implications of doing array tracking with this?
- Assuming those performance implications are relatively minimal… can we implement this same behavior for POJOs, given how they're often used as dictionaries? If not, what's the difference?
@chriskrycho great questions! I'm going to do a bit of of a dive here to add some context, and explain the purpose of this API. We think that it may make sense to make it public at some point in the future, which is why the feature is named What is
|
@pzuraq We need to address this underlying issue, but I'm not sure this is exactly the right solution. Something seems off about this solution but I don't have time to dig into it right now. It's definitely not a bugfix, though. |
@wycats I considered it a bugfix since the We could scope this down to just checking if the value is an array/EmberArray for now, if we want to only focus on the array use cases and not add a generic internal API. |
7f341da
to
5fcc7e9
Compare
b7c96f8
to
1770c33
Compare
This PR adds autotracking of arrays and Ember arrays. This is useful for arrays in particular because they have an indeterminate number of tracked values, and it would be impossible to actually track all values manually. The strategy is to check any value that we get back from a tracked getter method and see if it's an array - if so, we push its tag directly onto the stack. Later on, if the array is marked dirty via `pushObject` or `shiftObject` or some other method, it'll invalidate the autotrack stack that included the array. The one scenario this strategy does _not_ work with current is _creating_ an array in a getter dynamically: ```js class Foo { get bar() { if (!this._bar) { this._bar = []; } return this._bar; } } ``` In practice, these cases should be fairly rare, since getters should typically represent derived state from some source value, which would be tracked. Computed properties/cached getters _do_ add the tag for the object as well, since their state can be more long lived, so this won't be an issue for those.
1770c33
to
8c4c3e0
Compare
This PR adds autotracking of arrays and Ember arrays. This is useful for
arrays in particular because they have an indeterminate number of
tracked values, and it would be impossible to actually track all values
manually. The strategy is to check any value that we get back from a
tracked getter method and see if it's an array - if so, we push its tag
directly onto the stack. Later on, if the array is marked dirty via
pushObject
orshiftObject
or some other method, it'll invalidate theautotrack stack that included the array.
The one scenario this strategy does not work with current is
creating an array in a getter dynamically:
In practice, these cases should be fairly rare, since getters should
typically represent derived state from some source value, which would
be tracked. Computed properties/cached getters do add the tag for the
object as well, since their state can be more long lived, so this won't
be an issue for those.