-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[CHORE] removes deprecated Store.filter feature #5437
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.
LGTM
@@ -514,6 +358,8 @@ function updateLiveRecordArray(array, internalModels) { | |||
|
|||
if (modelsToAdd.length > 0) { array._pushInternalModels(modelsToAdd); } | |||
if (modelsToRemove.length > 0) { array._removeInternalModels(modelsToRemove); } | |||
|
|||
return modelsToAdd || modelsToRemove; |
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.
Why do we return here now?
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.
A minor perf optimization for the addon-based filter implementation. This essentially lets us know when none of the internal model changes were an addition or a remove, which tells the addon that it definitely needs to signal a recalculation. Without this we blindly signal, leading to the occasional double notification.
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.
I'll leave a note as to why and also that it can be removed in the follow up refactor to eliminate the unnecessary record-array-manager notifications post 3.5
Are there tests that assert the record array manager behavior the addon relies on? |
@igorT I thought there was one (but without a description), but looking back it seems not.I'll add one with a note as to purpose. |
b62897e
to
ec9acba
Compare
@igorT added tests and code comments |
ec9acba
to
9297ccc
Compare
See ember-data/ember-data-filter#12 for path forward for folks still using this long-since gated feature.
This approach relies on the fact that we still notify
record-array-manager
on any change to aninternal-model
, even if it isn't an addition or removal from aLiveRecordArray
. Once the deprecation cycle is complete (3.5
target), we can do an additional cleanup phase in which we no longer will be required to noisily notify therecord-array-manager
of every change.Associated deprecation RFC: emberjs/rfcs#326