facebook / relay

Relay is a JavaScript framework for building data-driven React applications.
https://relay.dev
MIT License
18.41k stars 1.83k forks source link

Support code-splitting mutation operations with useMutation #4827

Open alex-statsig opened 2 weeks ago

alex-statsig commented 2 weeks ago

Almost all mutations are not needed for the initial render of a page, often being triggered by some delayed user interaction like a button click. However, it is currently hard to avoid bloating bundles with the mutation nodes when using the built-in useMutation hook. The mutation nodes contain JS proportional to the mutation document size (and thus potentially proportional to the page size if the page's data is included as a fragment) in the query text and operation/fragment fields.

There's two main problems I've run into trying to solve this:

  1. useMutation requires a ConcreteRequest (GraphQLTaggedNode) as an argument. Since hooks cannot be called conditionally, this means the operation must be imported before the component using the hook can render. This makes it tricky to code-split the mutation's node from the component which triggers it. A possible fix here is to accept a Promise/JSResource for the node in the hook. When the commit function is called, it would chain off the promise/resource (potentially delaying the mutation request slightly if the resource hasn't resolved). This resource could be optimistically prefetched, or deferred until the user interacts (likely up to the caller to decide based on the tradeoff of mutation likelihood and desired speed of issuing the mutation)
  2. While it is possible to use persisted queries to avoid including the entire operation text in the bundle, the ConcreteRequest's fragment/operation fields still contains the AST of the mutation. This is inevitable to parse the response of the mutation, but not to kick off the mutation. The @preloadable annotation does allow generating a separate file containing just the parameters needed to issue the request, which can already be used to decrease bundle sizes with useQueryLoader+usePreloadedQuery. A possible fix here is to extend support to mutations, and add a "useDeferredMutation" hook which accepts Mutation$Parameters (PreloadableConcreteRequest), as well as a promise/JSResource for the actual mutation node (similar to useQueryLoader, or perhaps useEntryPointLoader)

While it would be possible to roll my own version of useMutation which allows this (the logic doesn't seem too complicated), I think it would be beneficial to provide first-class support to encourage this pattern. I'd be happy to help contribute something for this, but would like some confirmation that these are the right paths to go down

captbaritone commented 2 weeks ago

Yes! This is a much needed ability, I'm fully in favor of finding a way for us to support this. In fact, it may be that this should be the recommended default way to use mutations. Given that, we should think carefully about what would be the best API here to make it easy to get right and low-friction.

One significant challenge is that today the babel transform blindly converts the graphql tagged template literal into a require of the generated artifact which is the bit we want to avoid eagerly loading. And yet, defining the mutation directly in the hook that's going to dispatch it is the best low-friction developer experience.

I could also see a few options around when we load the artifact:

  1. Fully lazily (when the mutation is dispatched by the user)
  2. Via hints from the user (we could expose a function from the hook to fetch the artifact and the user could call that, for example, when the user mouses over the button that would trigger the mutation)
  3. As soon as possible. The artifact could be omitted from the main bundle, but fetched as soon as the main bundle executes.

There are a lot of options here, so probably the next step would be to brainstorm. What timezone are you in? Would you be interested in attending one of our design discussion meetings to discuss what options we have here and decide what the next concrete steps would be?

I think this is really important work and would love to help support you get this feature added to Relay!

alex-statsig commented 1 week ago

One significant challenge is that today the babel transform blindly converts the graphql tagged template literal into a require of the generated artifact which is the bit we want to avoid eagerly loading. And yet, defining the mutation directly in the hook that's going to dispatch it is the best low-friction developer experience.

Ah yeah I hadn't factored this in. Needing to define that in a separate file is definitely tedious (on top of likely defining a hook per mutation in its own file to capture common response handling logic)

There are a lot of options here, so probably the next step would be to brainstorm. What timezone are you in? Would you be interested in attending one of our design discussion meetings to discuss what options we have here and decide what the next concrete steps would be?

I am in PT, I'd be happy to join a design discussion meeting on this!