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

feat(cache): add cache operator #1320

Merged
merged 3 commits into from
Feb 10, 2016
Merged

feat(cache): add cache operator #1320

merged 3 commits into from
Feb 10, 2016

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Feb 9, 2016

The cache operator is effectively publishReplay and refCount.

The main reasons it's not named shareReplay:

  1. "share" no longer means "publishTypeX + refCount" in RxJS 5. It's not publish().refCount() it's a replayable refCounted observable. So shareReplay doesn't make sense, as this has little to do with share.
  2. cache is more terse and descriptive to the general user. What does it do? It caches values. Does it share a replay? sure. Will new users ever search for that... probably not? Debatable.

Bike shedding is bike shedding. I think it's cleaner.

(ATTN: @staltz ... I think there's probably a better way to write the tests than what I did)

@benlesh
Copy link
Member Author

benlesh commented Feb 9, 2016

Related #839

@staltz
Copy link
Member

staltz commented Feb 9, 2016

@Blesh tests look alright, except there could be more of them (but that's debatable since we already have tests for publishReplay).

@benlesh
Copy link
Member Author

benlesh commented Feb 9, 2016

@staltz that's what I was thinking, I tried to add tests I thought were specific to the behavior, but I can add a few more and repeat those other tests I suppose.

@staltz
Copy link
Member

staltz commented Feb 9, 2016

PS: what happened to the "advanced cache invalidation attributes" ideas for this operator?

@benlesh
Copy link
Member Author

benlesh commented Feb 9, 2016

PS: what happened to the "advanced cache invalidation attributes" ideas for this operator?

Lack of interest, mostly.

@benlesh
Copy link
Member Author

benlesh commented Feb 9, 2016

I've gotten some feedback from a few people that perhaps this operator requires some sisters like:

cacheUntil(observable), cacheWhile(fn), cacheTime(ms), and cacheCount(n)

@benlesh
Copy link
Member Author

benlesh commented Feb 10, 2016

Instead of the above operators what about a config object?

cache({ time: 1000, count: 5 })... etc. Makes the operator more readable I think, and opens the door for other types of cache eviction in the future without breaking the API or needing to add new operators.

@benlesh
Copy link
Member Author

benlesh commented Feb 10, 2016

Alternatively, how do people feel about reversing the order of arguments? cache(ms, count, scheduler)?

@kwonoj
Copy link
Member

kwonoj commented Feb 10, 2016

maybe name it to cacheTime(timespan, ?maxCount, ?scheduler) and introduce pair operator such as cacheUntil, etcs? Thought came from operators like buffer, window splits.

@justinwoo
Copy link
Contributor

I said this on twitter, but I think most people want caching by count, usually just 1 or 2, even. Maybe best if that's the first param if this will take multiple params (but I think splitting this into multiple operators would be nicer...)

But if you change the options hash to allow more/less/different values, then that's a change in the type of object you can send through, making it messy.

@benlesh benlesh merged commit 00db1ca into ReactiveX:master Feb 10, 2016
@staltz
Copy link
Member

staltz commented Feb 13, 2016

cache({ time: 1000, count: 5 })

It's a neat API, we should consider it for the future, but right now it's not consistent with the rest. cache API as it was merged is consistent, also cacheCount, cacheTime would be consistent.

@benlesh benlesh mentioned this pull request Mar 21, 2016
@benlesh benlesh deleted the cache-feat branch April 27, 2016 17:14
@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants