-
-
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
[BREAKING BUGFIX] eliminate infrastructure for filter record arrays #6702
Conversation
Asset Size Report for 579718f EmberData shrank by 473.0 B (83.0 B compressed)If any packages had changed sizes they would be listed here. Changeset
Full Asset Analysis
|
There are 6 travis failures, all in one module, that are not the usual travis-ci flake. Will investigate. |
Fixed travis upstream, they had implemented a custom filter that (like the deprecated and removed filter) depended on this infra. They now use computed properties observing peekAll's result with more typical |
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, although I'd describe the potential breakage a bit differently.
It may have been the mechanism filters were previously implemented with, but at this point we are over-eagerly emitting []
change notifications and that aspect of the cleanup amounts to a bugfix.
While there is some risk to apps that fixing this will cause minor breakages, the fixes to app code when this occurs are simple, and in apps >= ember-source 3.13 there may not even be this risk due to tracked and autotracking. After considering whether this is a risk we should take, we're going to merge this, as the effort to attempt a deprecation would be large and the refactoring and improvements it hinders are big. |
We deprecated filters ages ago and completed killing them off after 3.4. This is the scheduled removal of some secret notification infra we had left in place to keep them working a tad longer if needed.
This infrastructure had the unintended bug of making us notify array
length
and[]
over-eagerly.Due to this, the change MAY break apps that were using observers or computed properties on the
length
or[]
properties ofRecordArray
s when intending to observe changes to properties on records inside these arrays.For such cases there is an easy refactor to more standard Ember computed property usage ala
allRecords.@each.{aProp,bProp}
. This risk may be lower in versions of Ember in which @Tracked is in use.