-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(added cacheConfig in RequestDescriptor) #3070
Conversation
This change allows you to associate cache configurations directly in the request object. In this way, once the operation has been created, it is possible to access all the information necessary to create a request in the network.
…ction if retry with a different cache configuration is requested, a new operation is created
hi @kassens, |
friendly ping @jstejada @josephsavona |
Overall this makes a lot of sense to me. I'm still skeptical of per-query TTL, but overall this could be a clean way to allow alternate store implementations to experiment with such features. My main concern is what to do about the RequestIdentifier, which is intended to identify and de-duplicate requests for the same information. There are various places in Relay where we de-duplicate queries based on this identifier, and we need to decide whether the CacheConfig should be part of that or not. I am inclined to agree w the implementation here and say no - the CacheConfig isn't part of the request id. But we need to think through this and make sure that doesn't cause problems for the intended use-case here. Thoughts? |
I am of the same opinion, because the request is always the same and leaving the identifier the same I have the possibility to recognize that the query is already retained in the store or it is still pending in the network |
@josephsavona I just resolved the conflicts, any ETA on this? |
friendly ping
|
cc @jstejada and @rbalicki2 |
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.
@rbalicki2 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Hi @rbalicki2, any update or ETA on this PR? thanks 👍 |
@rbalicki2 merged this pull request in eb256a3. |
This change allows you to associate cache configurations directly in the request object.
In this way, once the operation has been created, it is possible to access all the information necessary to create a request in the network.
In addition, this modification allows you to add parameters to the CacheConfig object and easily use this information within Relay.
An example of a use case is to allow the configuration of a different TTL (bb88806) in the queries. So just add the TTL parameter inside CacheConfig and use that information inside the retain function without having to change its interface.