Skip to content
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

Merged
merged 1 commit into from Apr 19, 2018

Conversation

runspired
Copy link
Contributor

@runspired runspired commented Apr 17, 2018

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 an internal-model, even if it isn't an addition or removal from a LiveRecordArray. 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 the record-array-manager of every change.

Associated deprecation RFC: emberjs/rfcs#326

Copy link
Member

@bmac bmac left a 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;
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

@igorT
Copy link
Member

igorT commented Apr 18, 2018

Are there tests that assert the record array manager behavior the addon relies on?

@runspired
Copy link
Contributor Author

@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.

@runspired
Copy link
Contributor Author

@igorT added tests and code comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants