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

Implement ability to directly prime the cache #18

Merged
merged 3 commits into from
Apr 5, 2016

Conversation

ryanspradlin
Copy link
Contributor

This PR implements a new prime(key, value) API that directly primes the cache with a given key and value.

This is mentioned in #2 as a way to enable building a two-way cache, but I think a more basic motivation for this API would simply be to allow cache pre-warming. Another use case I have for this is to allow creating a DataLoader subclass that implements loadAll() (https://gist.github.com/ryanspradlin/1e951a4dcf737e3fb194).

@ryanspradlin
Copy link
Contributor Author

Looks like this build is failing right now just due to the babel-eslint/escope issue: babel/babel-eslint#243 (fixed in newest babel-eslint).

@ryanspradlin
Copy link
Contributor Author

Here's a PR to upgrade the babel-eslint dependency: #19

@ryanspradlin
Copy link
Contributor Author

Friendly bump -- any thoughts on this PR?

@leebyron
Copy link
Contributor

Sorry for the delay on this. Last time we discussed this we were concerned about the race-condition case where a current request is already outbound when prime is called. In other words, what should the behavior of prime be the promise cache already has a value in it for the given key? It could be possible for a get, prime, get call to happen in that order such that the two subsequent gets do not result in the same value, which could cause issues.

I think the uses of building a two-index cache and pre-warming a cache are in some ways at odds with each other.

Maybe the right way is to do nothing if they key already exists? That way you would have to explicitly declare if you wanted to forcefully prime the cache with loader.clear(key).prime(key, value).

@ryanspradlin
Copy link
Contributor Author

Apologies for the delay on my end this time. Thanks for the feedback -- that makes sense, and your suggestion sounds great to me. I've updated the PR to reflect the new logic (priming an existing key will do nothing), and updated the documentation and tests to match.

Thanks!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5e21df6 on ryanspradlin:features/prime into a57c44f on facebook:master.

@leebyron
Copy link
Contributor

leebyron commented Apr 5, 2016

Excellent work!

@leebyron leebyron merged commit 9b025ee into graphql:master Apr 5, 2016
leebyron added a commit that referenced this pull request Apr 5, 2016
@leebyron leebyron mentioned this pull request Apr 5, 2016
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