-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
findComponentRoot(): Unable to find element error thrown from ReactDOM.findDOMNode called from componentWillMount on new renders #4999
Comments
I don't think this change was intentional. What do you expect findDOMNode to return inside componentWillMount? What did it return before? |
Here's a snip of the code that broke: componentWillMount() {
if ( isBrowser ) {
// jshint browser: true
var $ = require('jquery'),
ReactDOM = require('react-dom');
// Test for File API based upload support
var FileAPI = window.File && window.FileReader && window.FileList && window.Blob,
XHR2 = window.XMLHttpRequest && ('upload' in new XMLHttpRequest()),
canvas = document.createElement('canvas');
if ( FileAPI && window.URL && XHR2 && canvas.getContext && canvas.toBlob ) {
this.setState({fileAPI: true});
if ( 'ondragover' in ReactDOM.findDOMNode(this) ) {
$(document)
.on('dragover', this.onDragOver)
.on('drop', this.onDrop);
this.setState({dnd: true});
}
}
// ...
}
} This case was simply getting the rendered node with |
In terms of expectations, it seems like findDOMNode SHOULD throw if invoked before mounting, shouldn't it? The component isn't mounted yet, so there wouldn't ever be a DOM node, or would there? |
Perhaps. But it previously worked and the error points to the wrong cause. So at least the message needs updating. |
Yeah, I'd never object to having a more helpful error message. Just wanted to confirm though, because your sample code indicates you were somehow using it before mounting, which seemed... odd. Just wanted to confirm I wasn't missing anything obvious. Task is to add a new error message for using findDOMNode on an unmounted component. Probably a #goodfirstbug? |
@dantman What did it do before? I can assure you that it did not return a DOM node since none has been created yet. |
Maybe there are circumstances around what you were doing (eg, maybe something with the same exact key was not properly cleared from the cache after a previous render) but in the simple case, |
This doesn't look like a regression so I'm unmarking 0.14. |
Perhaps this would be useful for server-side rendering? Say you're mounting a text input box. It's possible the user has typed something before React has finished initializing. Maybe you want to check for any existing user input to copy into the state? |
Yes, it's possible. But React's event handlers aren't attached until just before componentDidMount is called, so componentDidMount would be a better place to check anyway. In any case, we haven't hammered out the exact details of how supporting that use case (responding to events that happened in between server rendering and client mounting) should work. |
I was thinking that any user-entered data would be lost by the time you've reached componentDidMount(). Once the component is mounted, wouldn't render() already have wiped out the input? |
No, React basically doesn't ever read from the DOM and in server rendering it just leaves the DOM as-is as long as the checksum (of the original generated markup) matches. We might reconsider exactly how this works but that's what currently happens. |
Okay. So really, the only hole would be if the checksum is different, a less common but still valuable case. Eg having some slightly different DOM initially for a non-JS flow. (sidenote: I care about non-JS to help users recover and continue from partial loads on mobile. No-script users can go away :) ). But if a checksum is different, it's probably not possible to reliable form dom node references. You wouldn't know what the previous render() did to map that out. Perhaps for that case, just drop your own id on an element and query for it in getInitialState().. For form elements where the checksums all lined up, you could handle this as part of the existing lifecycle. Fire an onChange with the difference. That would give a poor developer experience though if it only worked some of the time. Maybe something like "serverId" on form elements to indicate initial state can be pulled for an onChange from a server-rendered node... (sorry if I went completely off-topic here) |
@JaRail @spicyj That issue is #2585 and #4293. I gave a suggestion on how I think dealing with implicitly autofilled data should be handled in the first bug. @JaRail #1979 is about differences in what is rendered on server and on client. The idea so far to deal with checksums is for the "am I on the server or client" boolean to be in context and for client code rendering from a server-rendered dom to first render with that set to server (to sync up with the server dom) and then re-render with it set to client (to add on the client-only stuff). I just realized why this actually worked for me. @zpao was half right. When I first render on the client, I render on top of a dom previously rendered by the server. So the node in question did already exist. I started getting the error not when I updated react. But when after updating I added in some responsive layout code that caused this component to get re-rendered from scratch after the page loaded. So, not a regression. But still could use a better error message. Since the mistake of thinking this works and then getting this error later on in development is not a random quirk but something that is a reasonable byproduct of using React's server-rendering. |
This error appears to be fixed in master at
null to match the semantics of a node that isn't found.
I have something that emits a warning, but can't guarantee that this would execute under the |
If this is fixed in master, let's just add a test to make sure that it continues working. Any interest in sending a PR? |
I recently ran into this error after updating to 0.14.0-beta3 from alpha3.
After a pile of hunting, thinking it might be related to some other changes I made (I didn't test this component right after the update and made some other major changes), and even discarding a
ReusableBlockMixin
experiment I thought was at fault. I finally realized that this error was called from aReactDOM.findDOMNode
call which was made insidecomponentWillMount
.This was valid in 0.13 and the alphas of 0.14 and the change wasn't documented.
Some more context to this error message would be appreciated. The current wording can mislead developers into looking for bugs into all the wrong places. Since calling
findDOMNode
has nothing do do with mutating of the dom. It's because of changes to when the DOM is setup.The text was updated successfully, but these errors were encountered: