Add Relay context to Relay containers #683
Conversation
|
@denvned My apologies for the delay in reviewing this. I'll write up more detailed thoughts in the morning, but for now I just wanted to thank you for your contribution :-) |
| /** | ||
| * @internal | ||
| */ | ||
| static resolveProps( |
There was a problem hiding this comment.
What necessitates this change? Mutations will never need a RelayContext instance - the changes here seem focused on passing in the instance of RelayStoreData instead of using the shared one.
Let's make a new PR with the combined changes to RelayMutation and RelayMutationQueue.
|
At a high level, this is definitely the direction we want to go and is a good step towards implementing #558. However, it will be difficult to import and land this PR in the current form due to the sheer volume of code changes, the impact on public APIs, and the risk inherent in significantly changing core functionality. The only practical way to land this will be to split it up: first prepare the big change without affecting public APIs, then add one final PR to make the cutover. This will a) make it much easier to review, b) make the dependencies between changes explicit and c) isolate blame in the case something does go wrong. Kent Beck summed up this approach more succinctly than I can: "for each desired change, make the change easy (warning: this may be hard), then make the easy change" - https://twitter.com/KentBeck/status/250733358307500032. Here's how that might look:
|
|
@josephsavona Thanks for the great review! I have started working on splitting this. Hope to submit the "prepare the big change" PR tomorrow. |
|
@josephsavona Should I submit the PR making |
|
Probably better to wait until the other PR is merged so you can base your work on that. |
|
Excellent work! Though, I'm surprised this does not touch the query caches (https://github.com/facebook/relay/search?utf8=%E2%9C%93&q=querycache) Their Is this out of scope for this pull request? |
@fabiosantoscode Yup, those query caches are unrelated to this PR. If you're seeing a behavior that seems incorrect or that might be a bug, definitely file a separate PR so we can confirm. Thanks! |
|
I'll file an issue, but I only to confirmed this using the isomorphic-relay package (although the code that was leaking was relay, it had some things instrumented from the outside). |
Summary: > This PR is another step toward making all Relay state contextual (#558). Originally this was submitted as a part of #683, but josephsavona [suggested](#683 (comment)) to split it to smaller PRs. `RelayContext` will ultimately be the public API of Relay Core, exposing all methods required to implement declarative APIs such as `RelayRootContainer` and `RelayContainer`, and will manage its own private instance of `RelayStoreData` (which will be an implementation detail). This diff is a first step in that direction: - Creates the `RelayContext` class - Changes `RelayStore` to be a (singleton) instance of the above, using the singleton `RelayStoreData` to ensure that callers of `RelayStoreData.getDefaultInstance()` get access to the same information as that accessed by via `RelayStore` - i.e., that all existing apps keep working as-is. Closes #698 Reviewed By: yungsters Differential Revision: D2784307 fb-gh-sync-id: e1764f9e0ef1dc7a1d1016fe8fdfd9012e7e63d3
Summary: This PR is another step toward making all Relay state contextual (#558). Originally this was submitted as a part of #683, but josephsavona [suggested](#683 (comment)) to split it to smaller PRs. Closes #761 Reviewed By: wincent Differential Revision: D2863416 fb-gh-sync-id: f9907959aac71b14e54d2746b86927b6fd8b1952
Summary:== Original Description == This PR is another step toward making all Relay state contextual (#558). Originally this was submitted as a part of #683, but josephsavona [suggested](#683 (comment)) to split it to smaller PRs. == Summary of Changes == - Make `relayContext` a required prop on `RelayRenderer` - Change `RelayRootContainer` to pass `RelayStore` (a default instance of RelayContext) as the relayContext prop to its internal RelayRenderer - Update tests (in particular, checking that changing the relayContext prop on RelayRenderer works as expected) - Update products that use RelayRenderer directly to pass RelayStore as the Relay context Closes #810 Reviewed By: wincent Differential Revision: D3006237 Pulled By: josephsavona fb-gh-sync-id: 6cf774f1ee5a81ec9d3c5e499f0d1e25fe3befc5 shipit-source-id: 6cf774f1ee5a81ec9d3c5e499f0d1e25fe3befc5
Summary:== Original Description == This PR is another step toward making all Relay state contextual (#558). Originally this was submitted as a part of #683, but josephsavona [suggested](#683 (comment)) to split it to smaller PRs. Currently, Relay resolve mutations props using the global `RelayStore`, and store them in the `props` instance field of `RelayMutation`. That makes it impossible to contextualize Relay mutations without changing the public API. After analyzing different alternatives, I have found that the probably cleanest solution is not to store resolved mutation props in an instance field, but to pass them as an argument to each of the mutation methods. That way mutations can be reused in different `RelayStoreData` contexts, and also they can be reused in the same context but at different times, because props are re-resolved at each mutation execution. == Summary of Changes == - Change `RelayMutation` constructor to not immediately resolve data for fragment props - Add `bindContext` method on RelayMutation, called when the mutation is committed on a specific RelayContext - This necessitated adding the `read` method to `RelayContextInterface`, and stubbing this method on alternative context implementations. - Updated mutation docs to mention this method. Closes #699 Reviewed By: yungsters Differential Revision: D2913334 Pulled By: josephsavona fb-gh-sync-id: 5051fddf068fc8bc7379bb55c8b1e4d97e33313c shipit-source-id: 5051fddf068fc8bc7379bb55c8b1e4d97e33313c
This PR is meant to finish the work started by @devknoll on contextualizing the Relay store. The previous his attempt on that was #603, but he didn't have enough time to finish that soon, so he gave me a permission to continue his work.
Escape from the global Relay store is very important for server side rendering (denvned/isomorphic-relay#6). It also solves the problem of resetting Relay store (#233).
I tried to make this PR as simple as possible to make the initial review easier. That's why there are few things not included yet:
relayContextproperty.I removed the global Relay store singleton altogether to make sure it is not used anywhere. But it might make sense to add placeholders for replaced API with deprecation messages, for backward compatibility (like here).