-
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
React-ids overriden after server-side rendering #2584
Comments
Update: Actually any state mutation that would alter the dom causes this error, on any component in the page, including for example typing in a text input that is bound to state. |
After more debugging, it appears server-rendered components do not increase the "nextReactRootIndex" variable, even though they do register themselves in containersByReactRootID. When the new component is being created it asks ReactInstanceHandles.createReactRootID() for the ID it should use, which in turn returns 0 (the id being occupied by the server rendered component) The new component then overrides the server rendered component, causing the reference to be lost. This overriding happens in the "registerContainer" function, though I would assume the bug originates in the index not being incremented for the server rendered component. |
I found a way to go around this problem. By not including the data-reactid in the server rendered component, the component is forced to ask "createReactRootIndex" for its index, incrementing the variable. I do however believe this is a bug and I hope someone more familiar with the code can find the reason. For now, removing the data-reactid in my server rendered components is sufficient. |
This worries me. I don't think React applications should be required to know about each other on the server side. For example, two react applications could be served from two separate services and both injected into the same page. I would expect that if the root nodes didn't conflict, the attach point node would be sufficient enough of an identifier. Numbering the applications in the DOM seems like a bad idea. |
This will probably sort it self out over time, but running multiple React instances is currently not supported. If you absolutely need to you can easily mod the React library to use a different attribute name for the different instances. |
@syranide i thought we randomized the root keys when server rendering? |
@zpao Oh right, yeah that does work for server-rendering actually (but not client-rendering...), but you have to be absolutely certain that there aren't race-conditions that could cause a checksum mismatch. Good call. Just remember that can't dynamically render any new roots client-side. I can't remember, but I think there are some (perhaps non-fatal) issues with the node cache too... wouldn't it actually cache all foreign nodes indefinitely due to events priming the cache? I'm uncertain about this... but it sounds familiar to me. I think the only really sane way of doing this is to have the different React instances use different attribute names for |
Right. Multiple Reacts on the client is going to be problematic but I didn't understand that to be the actual case here. We don't reassign keys on mounting existing markup, I thought we picked up the root key from the DOM (I think we have to for the checksums to match). Unless the "server side" generation here is actually in something like Phantom and we're determining if we randomize the root id based on execution environment instead of the code path. Then we would start at 0 on the server too. |
@zpao Oh, it seems I conflated JaRails comment with the issue itself. Now it makes sense :) Yeah, client-side reuse of server-rendered markup adopts the reactid in the DOM and should not increment "next index". The issue here is quite plainly that the server-rendered markup for some reason has non-randomized reactids... which is very weird because the checksum is there so it seems correctly done. Could Math.random() be monkey-patched/broken in the server environment? I'm not aware of any reason why React would not generate randomized reactids. |
@zpao It seems you're right, http://jsfiddle.net/ze6vc7L2/ I'm quite sure it wasn't broken like that before... |
@syranide No, it's unfortunately always been this way. On my infinite to-do list to fix. |
Am I losing any performance benefits by removing the data-reactid on the server-rendered component? |
@JohnyDays It will simply discard already rendered markup, so the client will rerender it from scratch and also clear whatever was on the screen (say if the user had begun to type in an input). EDIT: PS. You're not getting any warnings because React doesn't think it's rendered by React and silently discards it. |
So I basically lose any performance benefits gained, except for getting a faster initial page paint time. If it helps making a case for getting this fixed, my use case is having a global Loading component that i'm using on another node and is independent of the other react components. I can make a normal javascript class that manipulates the dom manually but I'd much rather use react for it. Also, react has been advertised as a framework that can be selectivelly applied in certain parts of an already developed product, and this issue limits that use case, when rendering those parts server-side.(albeit this is a limited use case) |
On a related note, would it not be positive to warn the user that they are mounting in a node which already contains nodes, and that they will be discarded upon mounting, when in development build? |
Seems like the easiest workaround is to patch the server-generated string with a random Id. http://jsfiddle.net/gvtm0q2h/4/ Seems to work okay. |
I'm guessing right now that's the best solution, thanks! I would have it be generated using the current unix epoch time, in order to have it be unique for certain. Is there any limit on the size of react-ids? |
http://jsfiddle.net/gvtm0q2h/5/ I patched your solution to use epoch time, and to also account for child nodes, which reference the parent react-id |
My problem with using times is that they can just as easily be duplicates in a fast system with low precision. For example, for (var i = 0; i < 10; i++) {console.log ((new Date()).getTime().toString());} I get mostly dupes with that. I'd rather just increase the 100 buffer and 10000 range if you're worried about collisions. Or have a getUniqueId() request-scoped (or whatever you need) singleton to manually increment on the server as the multiple react strings are patched. |
thanks for fixing for the child nodes though :) |
I just had another idea on how to fix this in the framework. Rather than a random Id, could renderToString() either use a placeholder Id or take an optional Id as input? If it started off looking like data-reactid=".x", the string would remain deterministic for the same inputs. I'm not sure if you have any hardcoded test strings but staying deterministic would be nice, particularly for affirming compatibility between different implementations down the road. render() would replace the placeholder Ids when it's mounted. I'm not sure if that would trigger a repaint in the browser. If not, it might be worth it to keep things deterministic and simple for the developer. |
Ran into the same issue today. Caused by two things: mounting a root node in the client side later on that hadn't been mounted on the server, but also we're doing server side rendering inside of a JSDOM instance. Currently working around the issue by just rewriting the implementation of createReactRootID in the JSDOM instance to be the randomized server side behavior. I'm curious though, is there a reason to just not use random always? |
We don't use IDs anymore so this can probably close. #10339 |
I have recently adapted my app to use server-side rendered markup and then reconciliate it with the client to increase loading speeds.
React correctly recognizes the rendered markup and doesn't actually touch the dom on the first rendering step.
However, when I interact(clicking, dragging, etc) with any server-side rendered react components I get the following error:
I understand this is a side-effect of dom mutation, however no such mutation has occured in the react element container.
Altering the state programatically(as opposed to interacting with clicks etc) does not cause these errors.
Here is the server-side rendered markup:
I am calling renderComponent on the client, on the dom node that contains that server-rendered markup.
E.g
Thanks for creating and maintaining this amazing UI framework!
The text was updated successfully, but these errors were encountered: