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

Get firstObject/lastObject notifications only when cached #14493

Merged
merged 1 commit into from
Oct 20, 2016

Conversation

stefanpenner
Copy link
Member

@stefanpenner stefanpenner commented Oct 19, 2016

Basically the contents of https://gist.github.com/mmun/877bad33158578a8609b

[fixes #5591]


changed:

cacheFor(this, 'firstObject');
cacheFor(this, 'lastObject');

to:

cacheFor.get(cache, 'firstObject');
cacheFor.get(cache, 'lastObject');

this lets us skip needing to re-fetch the cache (we just did above), but not leak the sentinel UNDEFINED value (which is why we have the methods in the first place.


  • Also switched to objectAt(array, index) and this to array
  • Also made length lazy unless lastObject needs accessing

@stefanpenner
Copy link
Member Author

@krisselden / @rwjblue should this ride canary or be a [BUGFIX] of some sort?

@rwjblue
Copy link
Member

rwjblue commented Oct 19, 2016

I think we can go with [BUGFIX beta], we are still super early in the beta cycle...

@stefanpenner stefanpenner force-pushed the pr-5591-rebased branch 3 times, most recently from 1074c9f to 3c4c08f Compare October 19, 2016 22:29
@stefanpenner
Copy link
Member Author

stefanpenner commented Oct 19, 2016

going to sleep, i'll fix tests and address feedback when I wake up.

@stefanpenner
Copy link
Member Author

stefanpenner commented Oct 19, 2016

@krisselden should we:

  • go all the way (like the PR currently does) and compare firstObject to the last firstObject we new about before broadcasting the changes(more aligned with our setter strategy),
  • or should we merely broadcast if the cache has been set before (more aligned with our existing CP DK strategy),
  • or should we broads if the first index or last index was part of the changeset?

@krisselden
Copy link
Contributor

krisselden commented Oct 20, 2016

@stefanpenner it is currently already more like the set() strategy, it was just doing it before firstObject and lastObject were ever consumed which is what this is addressing. I lean toward landing it as is and proposing considering the other changes after

@stefanpenner
Copy link
Member Author

@krisselden sounds good, we can always switch to the more CP style by checking affected array offsets in the future.

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.

4 participants