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

React 16.10 broke Next.js/SSR applications #16938

Closed
Timer opened this issue Sep 28, 2019 · 10 comments · Fixed by #16943
Closed

React 16.10 broke Next.js/SSR applications #16938

Timer opened this issue Sep 28, 2019 · 10 comments · Fixed by #16943
Assignees

Comments

@Timer
Copy link

Timer commented Sep 28, 2019

Do you want to request a feature or report a bug?
Bug

What is the current behavior?

React 16.10.0 has broken all Next.js applications (and potentially other SSR solutions).

It appears you cannot hydrate in conjunction with a client-side <Suspense> component.

Expected to have a hydrated suspense instance. This error is likely caused by a bug in React. Please file an issue.

image

CodeSandbox: https://codesandbox.io/s/i66g1

What is the expected behavior?

Not entirely sure -- I'm opening this issue to discuss. The provided example worked in React 16.9.0 (and prior releases containing Suspense).

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

react@16.10.0/react-dom@16.10.0 is broken.
react@16.9.0/react-dom@16.9.0 works.

@Timer Timer changed the title Next.js incompatible with React 16.10 React 16.10 broke Next.js/SSR applications Sep 28, 2019
@acdlite
Copy link
Collaborator

acdlite commented Sep 28, 2019

Thanks for reporting! We'll have a fix out soon; in the meantime, please downgrade back to 16.9.0.

@acdlite
Copy link
Collaborator

acdlite commented Sep 28, 2019

I believe technically we don't support conditionally rendering a Suspense boundary when server rendering. It just happened to (accidentally) work before.

We'll either fix so it matches the old behavior or provide a more graceful error or fallback behavior.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Sep 28, 2019

The issue is that the error only occurred on the server. So if you conditionally rendered it, you’d miss (intentionally?) the error we logged to let you know not to use Suspense with SSR. The issue is that the accidental semantics this lead to, on the client, is not actually the semantics we want SSR Suspense to have. That’s why it was not yet available for SSR.

We’ll at least need this fallback behavior to behave reasonably when the server renders the boundary. https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberBeginWork.js#L1970

Now the question is if we can preserve the old accidental behavior if the server was conditionally excluding the boundary on the server. And if that’s even a good idea.

@iksent
Copy link

iksent commented Sep 28, 2019

I've just found that error Minified React error #317 today only in my production build of Nextjs. There was no such error yesterday.
But I am having React ^16.8.6 and didn't update any packages.
Is it possible?

@threepointone
Copy link
Contributor

@iksent please file a separate issue. I’d recommend including a reproducing example on codesandbox or as a git repo.

@phamducquanptit
Copy link

phamducquanptit commented Sep 28, 2019

I got the same error on version react^16.8.4 | react-dom^16.8.4 for my production build of Nextjs. I tried upgrade version react^16.9.0 | react-dom^16.9.0 but it still not resolved

Screen Shot 2019-09-28 at 23 05 56

@sebmarkbage
Copy link
Collaborator

The key issue here is that with the new model (behind the enableSuspenseServerRenderer flag), this would be considered a hydration error. Something caused a mismatch. We don't know if this is because the boundary was intentionally conditionally rendered, or if it was because something completely different was conditionally hydrated. E.g. consider this <>{c ? <div /> : null}<Suspense>...</Suspense></>. If it rendered the div on the server but not on the client, then the Suspense boundary would have a mismatch because it finds the div where it expected its own content.

I believe we will at least have to make this a hydration error log in DEV mode. So we'll need to find a fix in Next.js regardless. However, as a courtesy we can probably delay that log so there's time to coordinate a fix.

Now the other question is what the runtime semantics should be. In this case, we're already in an error state for hydration so it's not considered normal semantics. I see two possible semantics: 1) it attempts to hydrate using the content of the Suspense boundary. 2) it client-renderers the Suspense boundary content without hydrating and then continues which will likely delete the existing siblings that already had such content.

For (1) it gets quite strange if something suspends because in that case, it would try to hydrate using the fallback's content against the real content which could cause very strange effects.

Normally we favor deleting and regenerating content in these cases because it avoids risks like transferring state or clicks to the wrong target which can cause severe negative effects.

Therefore my preferred semantics here is actually that it regenerates on the client. This is not what Next.js currently assumes will happen though.

@acdlite
Copy link
Collaborator

acdlite commented Sep 28, 2019

We have a fix for this. I'll release momentarily as 16.10.1. Release branch is here: #16944

@acdlite
Copy link
Collaborator

acdlite commented Sep 28, 2019

This is now fixed in v16.10.1.

@threepointone
Copy link
Contributor

@damiangreen please file a new issue with a reproducing example, either on codesandbox, or as a git repo. Thank you!

lunaruan pushed a commit that referenced this issue Oct 16, 2019
Without the enableSuspenseServerRenderer flag there will never be a boundary match. Also when it is enabled, there might not be a boundary match if something was conditionally rendered by mistake.

With this PR it will now client render the content of a Suspense boundary in that case and issue a DEV only hydration warning. This is the only sound semantics for this case.

Unfortunately, landing this will once again break #16938. It will be less bad though because at least it'll just work by client rendering the content instead of hydrating and issue a DEV only warning.

However, we must land this before enabling the enableSuspenseServerRenderer flag since it does this anyway.

I did notice that we special case fallback={undefined} due to our unfortunate semantics for that. So technically a workaround that works is actually setting the fallback to undefined on the server and during hydration. Then flip it on only after hydration. That could be a workaround if you want to be able to have a Suspense boundary work only after hydration for some reason.

It's kind of unfortunate but at least those semantics are internally consistent. So I added a test for that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants