-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Unexpected warning when hydrating with portal and SSR #12615
Comments
I have a similar issue, which can also be solved by re-rendering on the client via setState. E.g. if I use it like this inside my main component:
Instead of getting this after hydration:
I get this:
And the warning is the same ( I'm not sure if this is the intended behavior. |
While hydrating portals is not supported (#13097), the message itself doesn't make sense. We'll need to investigate and fix it. |
@gaearon The purpose of fix is
right? I'm planning SSR Doc draft to website repo, so I have to be familiar to hydrate mechanism. |
Why are Portals not supported on SSR, even to render "nothing"? |
It doesn’t seem like rendering nothing is best — I don’t see what makes portal contents different that we don’t consider it worth server rendering. We’ll likely come back to this together with the revamp of server renderer for suspense. |
Portals are conceptually client-only components; used for things like modals that one generally wouldn’t want to render on the server - they also take a dom element, which renderToString doesn’t because there is no dom. I don’t see how there’s anything meaningful to be done with them on the server - i just also don’t see anything valuable from throwing. |
That's one use case but there are also others like sidebars and similar which are not necessarily client-only.
Right — in the current design. It could change. #8386 (comment)
The value of throwing is to explicitly acknowledge that portal won't work. You can easily work around it with |
I would like to fix this as my "good first issue" |
+1 |
Can I work on this issue? |
Sure. I guess you can. So how far have you gone? I'm actually thinking of picking it up too. |
|
Hi folks, any idea how I can find the file where the issue is, I can get it please some help. |
* Update build size * [CS] Clone container instead of new root concept The extra "root" concept is kind of unnecessary. Instead of having a mutable container even in the persistent mode, I'll instead make the container be immutable too and be cloned. Then the "commit" just becomes swapping the previous container for the new one. * Change the signature or persistence again We may need to clone without any updates, e.g. when the children are changed. Passing in the previous node is not enough to recycle since it won't have the up-to-date props and children. It's really only useful to for allocation pooling. * Implement persistent updates This forks the update path for host fibers. For mutation mode we mark them as having an effect. For persistence mode, we clone the stateNode with new props/children. Next I'll do HostRoot and HostPortal. * Refine protocol into a complete and commit phase finalizeContainerChildren will get called at the complete phase. replaceContainer will get called at commit. Also, drop the keepChildren flag. We'll never keep children as we'll never update a container if none of the children has changed. * Implement persistent updates of roots and portals These are both "containers". Normally we rely on placement/deletion effects to deal with insertions into the containers. In the persistent mode we need to clone the container and append all the changed children to it. I needed somewhere to store these new containers before they're committed so I added another field. * Commit persistent work at the end by swapping out the container * Unify cloneOrRecycle Originally I tried to make the recyclable instance nullable but Flow didn't like that and it's kind of sketchy since the instance type might not be nullable. However, the real difference which one we call is depending on whether they are equal. We can just offload that to the renderer. Most of them won't need to know about this at all since they'll always clone or just create new. The ones that do know now have to be careful to compare them so they don't reuse an existing instance but that's probably fine to simplify the implementation and API. * Add persistent noop renderer for testing * Add basic persistent tree test * Test bail out This adds a test for bailouts. This revealed a subtle bug. We don't set the return pointer when stepping into newly created fibers because there can only be one. However, since I'm reusing this mechanism for persistent updates, I'll need to set the return pointer because a bailed out tree won't have the right return pointer. * Test persistent text nodes Found another bug. * Add persistent portal test This creates a bit of an unfortunate feature testing in the unmount branch. That's because we want to trigger nested host deletions in portals in the mutation mode. * Don't consider container when determining portal identity Basically, we can't use the container to determine if we should keep identity and update an existing portal instead of recreate it. Because for persistent containers, there is no permanent identity. This makes it kind of strange to even use portals in this mode. It's probably more ideal to have another concept that has permanent identity rather than trying to swap out containers. * Clear portals when the portal is deleted When a portal gets deleted we need to create a new empty container and replace the current one with the empty one. * Add renderer mode flags for dead code elimination * Simplify ReactNoop fix * Add new type to the host config for persistent configs We need container to stay as the persistent identity of the root atom. So that we can refer to portals over time. Instead, I'll introduce a new type just to temporarily hold the children of a container until they're ready to be committed into the permanent container. Essentially, this is just a fancy array that is not an array so that the host can choose data structure/allocation for it. * Implement new hooks Now containers are singletons and instead their children swap. That way portals can use the container as part of their identity again. * Update build size and error codes * Address comment * Move new files to new location
Hey here is the partial pull request for this issue |
Anyone still on this? I'd love to take it on. |
What's the status of this? How could I reproduce it? |
Is this still an issue? |
I have not gotten a response on this issue either. |
I believe the status of this issue is still open. |
Can I take on this issue? |
Hello, is anyone working on this issue? If not I would like to take it. |
if no one is working on it, I can |
Is this still an issue? It seems like attempts were made to fix it, but the underlying problem is that some people actually want portals to work on server-side rendering? If that's the case can this issue be closed? |
Adding a div element should work fine `class Para extends React.Component {
Some Text
)
}
} `
|
My sidebar appears on the client after everything else, this flash is ugly. |
please let me know some good first issues that I can work on as I am new to open source |
|
Old issue but if someone sees it... I use the below hook to render the contents of the portal or other components if SSR would somehow fail to render it correctly, use sparingly, SSR is a good thing. "use client";
import { useEffect, useState } from "react";
export const useIsClient = () => {
const [isClient, setIsClient] = useState(false);
useEffect(() => {
setIsClient(true);
}, []);
return isClient;
}; ...
const isClient = useIsClient();
const retval =
isClient &&
<>
<Portal>
{......}
</Portal>
</>
return retval
... |
Do you want to request a feature or report a bug?
bug
What is the current behavior?
Given the following (simplified) snippet:
where
div#root
is a validdiv
that exists, the following error is shown when hydrating after SSR:Warning: Expected server HTML to contain a matching <div> in <span>
The warning goes away if I update the definition of
HoverMenu
to:I'd prefer not to do that because of the double rendering caused by
setState
incomponentDidMount
.I don't quite understand what that error is telling me. No
<div />
is rendered server-side in either case. The error is particularly confusing, as theHoverMenu
DOMdiv
is not even rendered inside a DOMspan
. (I wonder if this is happening becauseHoverMenu
is nested inside a Reactspan
.)What is the expected behavior?
No error is thrown. Or, at least that the error message is clearer.
Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
Chrome 65
React 16.2
(SSR through Next 5.1)
The text was updated successfully, but these errors were encountered: