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

[CLEANUP beta] Avoid before observers in ArrayProxy #16166

Merged
merged 1 commit into from
Jan 22, 2018

Conversation

mmun
Copy link
Member

@mmun mmun commented Jan 22, 2018

This PR removes one of two remaining internal usages of before observers (one in ArrayProxy and one in EachProxy).

In order to remain API compatible, we simulate arrayWillChange events by caching the values of the ArrayProxy upon accessing (e.g. when calling objectAt or getting the length). After arrayWillChange has been triggered we mark the cache as dirty and fire the arrayDidChange event. This will cause the cache to recompute on next access which will generally occur in response to the arrayDidChange event.

There are two further avenues of work:

  1. It should be possible to use the indexes in the _arrangedContentArrayDidChange hook to optimize the syncing of the cache to only the slice that changed.
  • It's a bit of tricky code to get this right, but I should probably just bite the bullet so that things like pushObject don't become O(N).
  1. It would be a good idea to make observation of arrangedContent lazy. You should only need to observe after you've accessed the value once, otherwise why would you care if it has changed?
  • When I tried implementing this it caused quite a few tests to fail in ember and ember-data. From a quick glance, most of these tests were synthetic unit tests and not indicative of how an array proxy would be used in an actual application. In any case, I prefer to make this timing change in a separate PR to make it easier to roll back if necessary.

@mmun mmun force-pushed the array-proxy-remove-before-observer branch from 3f3965e to 2ddcbc9 Compare January 22, 2018 17:33
@mmun
Copy link
Member Author

mmun commented Jan 22, 2018

I implemented an optimization to track the first dirty index and to only update the cache starting at that index. I tried tracking the dirty end index as well but it got messy and I don't think it is worth the effort. The current implementation should be suitable for all real world use cases.

this._cache = null;
this._dirtyStart = 0;
this._arrangedContent = null;
this._addArrangedContentArrayObsever();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't need to do this until there is something to invalidate

@rwjblue rwjblue merged commit cd20749 into master Jan 22, 2018
@rwjblue rwjblue deleted the array-proxy-remove-before-observer branch January 22, 2018 21:53
hjdivad added a commit to ember-m3/ember-m3 that referenced this pull request Feb 14, 2018
Previously we fired array change events on the proxy itself.  This is
incompatible with the changes in
emberjs/ember.js#16166 which depend on the
events firing on the underlying content.
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.

3 participants