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

Memoized child of Suspense component doesn't update when Context updates. #17356

Closed
jcarroll2007 opened this issue Nov 13, 2019 · 11 comments · Fixed by #19216
Closed

Memoized child of Suspense component doesn't update when Context updates. #17356

jcarroll2007 opened this issue Nov 13, 2019 · 11 comments · Fixed by #19216

Comments

@jcarroll2007
Copy link

Do you want to request a feature or report a bug?
bug (I think?)

What is the current behavior?

const [value, setValue] = useState("default");
return (
  <div className="App">
    <input value={value} onChange={e => setValue(e.target.value)} />
    <div>
      <Value.Provider value={value}>
        <Suspense fallback={<div>loading</div>}>
          <MemoizedChild />
         </Suspense>
       </Value.Provider>
     </div>
  </div>
)

When using a memoized functional component (MemoizedChild in above example) in conjunction with Context as a child of a React.Suspense component, there seems to be a bug in which MemoizedChild does not update when the context it uses changes. For the full example, see my codesandbox below.

In the codesandbox, if you change the value of the input, the new value is provided to the context which causes the hook used in MemoizedChild (useValue) to throw a promise. This flips Suspense to the fallback state and when the promise resolves MemoizedChild's state is not updated with the proper context value because (I'm assuming) the memoized value of MemoizedChild is the one that contained the previous context value and technically no props have changed, so that makes sense why it wouldn't have updated. However, this seems like it would be unexpected behavior.

https://codesandbox.io/s/react-suspense-maybe-bug-sznbk

What is the expected behavior?
I would expect that MemoizedChild would be re-rendered with the new provided value.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
I'm assuming all of them that contain Suspense and memo. So, since 16.8?

@aweary
Copy link
Contributor

aweary commented Nov 14, 2019

Thanks for the report @jcarroll2007, this does look like a bug. I reproduced with the experimental build as well and also verified that this only affects legacy mode (ReactDOM.render), Concurrent and Blocking Mode both behave as expected.

@aweary
Copy link
Contributor

aweary commented Nov 20, 2019

@gaearon can you take a look at this when you get a chance?

@overlookmotel
Copy link
Contributor

overlookmotel commented Feb 10, 2020

It appears to relate to using useContext() and throwing in the same component.

Here's the same example but with MemoizedChild split into two components - one with the useContext() hook, and a child inside it which throws.

https://codesandbox.io/s/react-suspense-maybe-bug-c62nh

This behaves as expected.

@gaearon
Copy link
Collaborator

gaearon commented Mar 30, 2020

Would anyone like to turn this into a failing test? This will get us closer to finding a fix.

@ankitamasand
Copy link

@gaearon can I take this up? I'll start with the failing test to figure out the bug!

@gaearon
Copy link
Collaborator

gaearon commented Mar 31, 2020

Sure!

@Tsdevendra1
Copy link
Contributor

Hi @gaearon, I've been looking into this a bit (first time exploring the react code base, so sorry if i've misunderstood anything) and this is what I've understood so far for the reproduction of the bug given in the first sandbox (https://codesandbox.io/s/react-suspense-maybe-bug-sznbk):

When the promise resolves after being thrown, didReceiveUpdate is false for the MemoizedChild when it is processed in updateFunctionComponent during the beginWork phase. This makes sense since from what I understand, didReceiveUpdate is conditional on a change of props or a change of context (might have missed some other cases) but neither of these happened after the promise resolved, hence it is correct that didReceiveUpdate is false. But due to didReceiveUpdate being false for the MemoizedChild, it means reconcileChildren is never called which is what would normally update the value (from what I understand).

As @overlookmotel pointed out, separating the context and throwing promise in two different components fixes the issue (https://codesandbox.io/s/react-suspense-maybe-bug-c62nh), but I believe in this case it is fixed because ChildOfMemoizedChild receives different props (the old version of resource which throws the promise and the new version which returns the value as intended, and as a result didReceiveUpdate is true and hence works as intended.

With this being said, if the didReceiveUpdate being false is the issue, I'm not entirely sure how to fix it since it seems more of a design decision than a bug to be able to recognise that a change occurred in this case.

@gaearon
Copy link
Collaborator

gaearon commented Apr 10, 2020

We need a failing test case to look further.

@acdlite
Copy link
Collaborator

acdlite commented Apr 10, 2020

I checked the test case submitted by @Tsdevendra1 and this is a bug in legacy mode Suspense: #18572 (comment)

@gaearon
Copy link
Collaborator

gaearon commented Jun 29, 2020

https://codesandbox.io/s/react-suspense-context-bug-cq2xq

Probably another example of this issue.

@gaearon
Copy link
Collaborator

gaearon commented Jun 30, 2020

Possible fix: #19216

gaearon added a commit to gaearon/react that referenced this issue Jun 30, 2020
gaearon added a commit that referenced this issue Jun 30, 2020
* Add a failing test for legacy Suspense blocking context updates in memo

* Add more test case coverage for variations of #17356

* Don't bailout after Suspending in Legacy Mode

Co-authored-by: Tharuka Devendra <tsdevendra1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants