facebook / relay

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

Unmounting one of multiple components that share a duplicate query deletes RecordSource records #2595

Open BJTerry opened 5 years ago

BJTerry commented 5 years ago

If you include two QueryRenderers on a page that have the same query, and then you remove one, it deletes the store data associated with the component because fetchQueryAndComputeStateFromProps in ReactRelayQueryRenderer.jsx doesn't properly handle duplicate requests from different components.

When fetchQueryAndComputeStateFromProps is called the first time, it calls fetch in ReactRelayQueryFetcher, which then calls execute. After the environment executes, eventually environment.retain is called in ReactRelayQueryFetcher.execute, which retains the returned data in the RelayMarkSweepStore (assuming you are using the normal store setup). The second time fetchQueryAndComputeStateFromProps is called it does not call execute because the query matches an in-flight request. Therefore, the data is not retained a second time in the RelayMarkSweepStore. When you unmount either component, it disposes the selector in RelayMarkSweepStore, which performs a gc, deleting the data for all remaining components.

I attempted to create a repro of this, but the glitch site linked in the Issues description is extremely out of date and doesn't support Relay Modern. The above was determined using Relay 1.7.0.

BJTerry commented 5 years ago

I've created a reproduction of the issue here:

https://github.com/BJTerry/relay-examples

  1. clone repo
  2. cd to todo
  3. yarn install yarn build yarn update-schema yarn start
  4. Go to localhost:3000
  5. I've created two copies of the component at page load. Above them there is a hard to see button that says Delete entry. Click that button.
  6. Click any checkbox on the remaining Todos.

You should see an error in the console like this:

TodoList.js:77 Uncaught TypeError: Cannot read property 'edges' of undefined
    at TodoList.renderTodos (TodoList.js:77)
    at TodoList.render (TodoList.js:101)
    at finishClassComponent (react-dom.development.js:13393)
    at updateClassComponent (react-dom.development.js:13356)
    at beginWork (react-dom.development.js:13945)
    at performUnitOfWork (react-dom.development.js:16249)
    at workLoop (react-dom.development.js:16287)
    at HTMLUnknownElement.callCallback (react-dom.development.js:145)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:195)
    at invokeGuardedCallback (react-dom.development.js:248)
react-dom.development.js:14389 The above error occurred in the <TodoList> component:
    in TodoList (created by ContainerConstructor)
    in ContainerConstructor (created by ForwardRef(Relay(TodoList)))
    in section (created by TodoApp)
    in div (created by TodoApp)
    in TodoApp (created by ContainerConstructor)
    in ContainerConstructor (created by ForwardRef(Relay(TodoApp)))
    in ReactRelayQueryRenderer (created by App)
    in div (created by App)
    in App

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

And the entire page crashes. I've created a video of the reproduction here: https://cl.ly/cdab28ee298f

josephsavona commented 5 years ago

cc @jstejada

josephsavona commented 5 years ago

Thanks for reporting this! As a workaround note that GC can be disabled with environment.getStore().__disableGC().

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.