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

ReactDOMServer.renderToString: presence of onClick handler causes errors on async update #5473

Closed
denvned opened this issue Nov 15, 2015 · 12 comments · Fixed by #7127
Closed

Comments

@denvned
Copy link

denvned commented Nov 15, 2015

The code

class Comp extends React.Component {
  componentWillMount() {
    setTimeout(() => {
      this.setState({});
    });
  }

  render() {
    return <div onClick={() => {}} />;
  }
}

ReactDOMServer.renderToString(<Comp />);

produces errors:

Warning: React can't find the root component node for data-reactid value .0. If you're seeing this message, it probably means that you've loaded two copies of React on the page. At this time, only a single copy of React can be loaded at a time.

Uncaught TypeError: Cannot read property 'firstChild' of undefined

But if I remove onClick handler the errors go away...

See demo: http://codepen.io/dened/pen/YyByYr

@denvned
Copy link
Author

denvned commented Nov 15, 2015

This line tries to get a node that is not mounted:

var node = ReactMount.getNode(id);

@jimfb
Copy link
Contributor

jimfb commented Nov 15, 2015

cc @spicyj and @syranide because: events.

actually, calling setState on SSR might not be legal/valid, is it?

@jimfb jimfb removed the Type: Bug label Nov 15, 2015
@sophiebits
Copy link
Collaborator

Yeah, I suppose setState should warn and do nothing. What did you expect would happen though?

@denvned
Copy link
Author

denvned commented Nov 15, 2015

Not only setState triggers this error. For example, forceUpdate also produces this.

I expect asynchronous updates to be ignored (with a warning) on components rendered using ReactDOMServer.renderToString.

@dominicenglish
Copy link

I also ran into this error when trying to update a codebase to make use of server-side rendering. There was a counter using setInterval initialised in the constructor for some reason that ended up trying to call setState.

I had trouble tracking the problem down as the error is pretty confusing in this case. Some sort of warning when setState is called would certainly help pinpoint the actual problem. I feel like it's a relatively easy mistake to make for those of us new to SSR.

@jimfb
Copy link
Contributor

jimfb commented Jan 15, 2016

In dev mode, we could mark a component as having been rendered using SSR. If setState or forceUpdate is ever called on such a component, print an intelligent warning.

@mdolbin
Copy link

mdolbin commented Jan 18, 2016

Hello, first attempts to dive into react sources here :) I took a look at similar issues but I still wonder if it's 'react-enough' way. I'm concerned about
if (this._reactInternalInstance && this._reactInternalInstance._isServerSideRendered) condition in particular. Am I on the right track or there is more convenient way to mark & check it? Thanks.

@jimfb
Copy link
Contributor

jimfb commented Jan 18, 2016

@mdolbin That looks reasonable to me.

@gaearon
Copy link
Collaborator

gaearon commented Jun 26, 2016

Looks like this is up for grabs again as #5879 did not progress.
Some info on how to implement it: #5879 (comment).
I updated the fiddle demonstrating the issue (the error message is different in 15.x but the cause is the same): http://codepen.io/gaearon/pen/jryXOj

@rricard
Copy link
Contributor

rricard commented Jun 26, 2016

I think I found a way to use ReactNoopUpdateQueue in ReactServerRenderingTransaction. That way we can do as @spicyj advised. I send you a PR as soon as possible, maybe tonight, maybe tomorrow!

@rricard
Copy link
Contributor

rricard commented Jun 27, 2016

Okay, trick question: how can I let the setState go through in componentWillMount if I use ReactNoopUpdateQueue ?

My change, for instance, make fail https://github.com/facebook/react/blob/master/src/renderers/dom/server/__tests__/ReactServerRendering-test.js#L383

@rricard
Copy link
Contributor

rricard commented Jun 27, 2016

Answering my own question: maybe during initial mount, don't use (yet) the ReactNoopUpdateQueue.

gaearon pushed a commit that referenced this issue Jul 4, 2016
This commit fixes #5473: ReactDOMServer.renderToString: presence of onClick
handler causes errors on async update

This commit performs the following changes:

- Adds a getUpdateQueue method to ReactServerRenderingTransaction,
  ReactReconcileTransaction, ReactNativeReconcileTransaction and
  ReactTestReconcileTransaction
- Make the ReactCompositeComponent call this getUpdateQueue instead of using
  ReactUpdateQueue that was unwanted at certain moments on server
- On ReactServerRenderingTransaction, dispatch ReactUpdateQueue's methods
  while rendering and warning methods afterwards. This is done through the new
  ReactServerUpdateQueue class
- Added a series of tests that mimics the case presented in #5473 with setState,
  forceUpdate and replaceState
- Add flow typechecking on concerned files
zpao pushed a commit that referenced this issue Jul 8, 2016
This commit fixes #5473: ReactDOMServer.renderToString: presence of onClick
handler causes errors on async update

This commit performs the following changes:

- Adds a getUpdateQueue method to ReactServerRenderingTransaction,
  ReactReconcileTransaction, ReactNativeReconcileTransaction and
  ReactTestReconcileTransaction
- Make the ReactCompositeComponent call this getUpdateQueue instead of using
  ReactUpdateQueue that was unwanted at certain moments on server
- On ReactServerRenderingTransaction, dispatch ReactUpdateQueue's methods
  while rendering and warning methods afterwards. This is done through the new
  ReactServerUpdateQueue class
- Added a series of tests that mimics the case presented in #5473 with setState,
  forceUpdate and replaceState
- Add flow typechecking on concerned files
(cherry picked from commit dbdddf1)
usmanajmal pushed a commit to usmanajmal/react that referenced this issue Jul 11, 2016
…ebook#7127)

This commit fixes facebook#5473: ReactDOMServer.renderToString: presence of onClick
handler causes errors on async update

This commit performs the following changes:

- Adds a getUpdateQueue method to ReactServerRenderingTransaction,
  ReactReconcileTransaction, ReactNativeReconcileTransaction and
  ReactTestReconcileTransaction
- Make the ReactCompositeComponent call this getUpdateQueue instead of using
  ReactUpdateQueue that was unwanted at certain moments on server
- On ReactServerRenderingTransaction, dispatch ReactUpdateQueue's methods
  while rendering and warning methods afterwards. This is done through the new
  ReactServerUpdateQueue class
- Added a series of tests that mimics the case presented in facebook#5473 with setState,
  forceUpdate and replaceState
- Add flow typechecking on concerned files
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