#5473 warn and ignore setState and forceUpdate calls if rendered on server #5879

mdolbin · 2016-01-19T05:21:54Z

As discussed in the #5473:

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.

…erver

jimfb · 2016-01-19T18:41:29Z

I think we can't ignore setState() (I know that was @spicyj's idea, but I think he was mistaken about this point). We should restore the 'allows setState in componentWillMount without using DOM' test, IMO. cc @spicyj for confirmation

Also, we should think through the cases where setState() is and isn't legal. I think setState within componentWillMount is legal but setState within a setTimeout is not legal. Maybe the easiest thing to do is have instance._isServerSideRendered point to an object which is created at the beginning of SSR ({warn: false}) and mutated at the end of render (set warn to true) to enable warnings on those components.

- isServerSideRendered is either 'false' or an object with a 'isAfterComponentWillMount' flag; - setState() method calls executed after componentWillMount() (e.g. setTimeout() callback) are ignored if component was rendered on server. All setState() calls during componentWillMount() are allowed; - all forceUpdate() method calls are ignored if component was rendered on server; - restored 'allows setState in componentWillMount without using DOM' test.

facebook-github-bot · 2016-01-20T20:49:12Z

@mdolbin updated the pull request.

mdolbin · 2016-01-20T22:01:30Z

@jimfb As far as I know the only use of state in case of server-side rendering is a one-time initial rendering itself, isn't it?
I think we shouldn't allow to change state during render which happens (immediately?) after componentWillMount so this bound seems to be in place.
The only thing I can think about is overriding some 'internal magic state properties' which is not the case, I hope.

jimfb · 2016-01-20T22:05:06Z

Yeah, I think I agree with you. cc @spicyj

jimfb · 2016-01-20T22:05:32Z

src/renderers/dom/server/__tests__/ReactServerRendering-test.js

+        'if component was rendered on server.';
+      jest.runAllTimers();
+      expect(setTimeout.mock.calls.length).toBe(1);
+      expect(console.error.calls.length).toBe(2);


We should probably add a boolean so we only emit the warning once (otherwise the user's logs might fill up with this message).

@jimfb I saw this pattern a lot of times, but it seems like it would be much easier to rework an abstract component as a whole if we know about all warnings at once (which can be delayed in time), instead of discovering them one by one. IMHO

Anyways, let me know if we still want to stick to the first warning only policy. I'll just add one more property to the _serverSideRendered object in that case

We generally stick to warn-once where possible/practical, because otherwise a warning that fires often can drown out a warning that only fires infrequently (which results in people not seeing the less-frequent warning).

Let's add a global boolean instead of using _serverSideRendered, since the latter will be thrown away each render.

Makes sense

jimfb · 2016-01-20T22:09:37Z

Overall, I think this is good. A couple of minor notes, then I think we can squash the commits together and merge.

sophiebits · 2016-01-20T22:13:08Z

Hmm. I thought we were going to start using ReactNoopUpdateQueue for server rendering instead of ReactUpdateQueue and in that case it would be easy to swap out an implementation that warns for server rendering. Really that would be a much cleaner solution than this.

Also, if we do go with this solution, please use ReactInstanceMap instead of _reactInternalInstance directly, like all of the surrounding code already does.

mdolbin · 2016-01-20T23:03:32Z

use ReactInstanceMap instead of _reactInternalInstance

Cool, thanks

- fire a single warning for all components; - this._reactInternalInstance => ReactInstanceMap.get(this); - _isServerSideRendered => _serverSideRendered; - updated tests.

facebook-github-bot · 2016-01-21T22:03:33Z

@mdolbin updated the pull request.

facebook-github-bot · 2016-01-22T19:33:24Z

@mdolbin updated the pull request.

mdolbin · 2016-01-31T23:12:08Z

@jimfb @spicyj let me know if there is something else I can do to improve my PR or if I should look for another solution. Thanks :)

jimfb · 2016-02-01T20:01:20Z

Looks good to me. I'll leave it open for another day in case there are any other feedbacks, and then we can merge.

jimfb · 2016-02-01T20:02:03Z

Oh, but please do squash the commits before we merge. You can do this using git rebase -i and then git push -f

sophiebits · 2016-02-01T22:02:44Z

There should not be logic about server rendering in src/isomorphic. It is a concept specific to the DOM renderer.

jimfb · 2016-02-24T23:12:36Z

@mdolbin Do you want to update as per @spicyj's comment?

mdolbin · 2016-02-24T23:53:01Z

@jimfb sure, sorry, a lot of work these days.
But still, I wonder how can I interrupt async calls to setState and forceUdpdate from the scope of dom renderers without any changes in ReactConponent itself

mdolbin · 2016-03-07T22:52:08Z

@jimfb @spicyj Hey, guys, can I have some kind of a hint, please? It seems like I really stuck here but there is no way I want to give up.

jimfb · 2016-05-03T01:47:53Z

@spicyj, any ideas on where you'd like the code to live?

sophiebits · 2016-05-03T03:08:58Z

Like I suggested originally, we should change to use ReactNoopUpdateQueue on the server, which would probably mean having queue come from the transaction (since that differs between client and server rendering). Also open to other ideas but that's what I suggested 3 months ago and it's still the best option that I see.

gaearon · 2016-06-26T18:45:07Z

I’m closing as this has been inactive for quite a few months but please leave another comment in #5473 and open a new PR if you’d like to work on this again! Thanks for your time.

evenstensberg · 2016-06-26T21:55:05Z

@gaearon I'm going to start to find a fix, but don't expect much. Do you mind providing as much info as you got on this issue? Been looking at how client side renders updates, were you thinking of involving ReactReconileTransaction to a more server friendly approach, or how do we want to accomplish noops? ( if I got the grasp right)

gaearon · 2016-06-26T23:06:20Z

I think the problem is that ReactCompositeComponent currently uses ReactUpdateQueue directly in several places. It passes it to public instances as the third updater argument so setState and forceUpdate calls fail during server rendering.

To fix the problem, instead of ReactUpdateQueue we would read the appropriate queue from the transaction argument by calling transaction.getUpdateQueue (new method). This would allow us to specify different values for it depending on the transaction type.

In ReactReconcileTransaction, getUpdateQueue() would return ReactUpdateQueue. In ReactServerRenderingTransaction, getUpdateQueue() would return ReactNoopUpdateQueue.

I hope this helps!

rricard · 2016-06-27T00:14:50Z

@ev1stensberg: sorry, seems that we started working on that at the same time. I have something on https://github.com/rricard/react/tree/fix-5473. I still have no tests so the PR is not complete yet (and I still need to fix two other failing tests), maybe we could work on that together, or if you can finish this PR faster than me, please feel free to use what I did!

evenstensberg · 2016-06-27T04:37:40Z

@rricard No worries, we are after the same thing here 😄 I'm busy 0400-1800 every day this summer, so I doubt I'll be done before you.

rricard · 2016-06-27T09:03:23Z

@ev1stensberg: cool! The PR is here if you are interested: #7127

evenstensberg · 2016-06-27T09:59:12Z

@rricard Looks like you manage well on your own!

#5473 warn and ignore setState and forceUpdate calls if rendered on s…

33337f0

…erver

facebook-github-bot added GH Review: review-needed CLA Signed labels Jan 19, 2016

mdolbin added 2 commits January 20, 2016 22:33

Merge remote-tracking branch 'upstream/master'

7280d27

jimfb reviewed Jan 20, 2016
View reviewed changes

#5473 warn-once, _serverSideRendered is either null or an object

7a96400

- fire a single warning for all components; - this._reactInternalInstance => ReactInstanceMap.get(this); - _isServerSideRendered => _serverSideRendered; - updated tests.

jimfb added GH Review: accepted and removed GH Review: review-needed labels Feb 1, 2016

jimfb self-assigned this Feb 1, 2016

sophiebits added GH Review: needs-revision and removed GH Review: accepted labels Feb 1, 2016

gaearon closed this Jun 26, 2016

gaearon mentioned this pull request Jun 26, 2016

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#5473 warn and ignore setState and forceUpdate calls if rendered on server #5879

#5473 warn and ignore setState and forceUpdate calls if rendered on server #5879

mdolbin commented Jan 19, 2016

jimfb commented Jan 19, 2016

facebook-github-bot commented Jan 20, 2016

mdolbin commented Jan 20, 2016

jimfb commented Jan 20, 2016

jimfb Jan 20, 2016

mdolbin Jan 20, 2016

jimfb Jan 20, 2016

mdolbin Jan 20, 2016

jimfb commented Jan 20, 2016

sophiebits commented Jan 20, 2016

mdolbin commented Jan 20, 2016

facebook-github-bot commented Jan 21, 2016

facebook-github-bot commented Jan 22, 2016

mdolbin commented Jan 31, 2016

jimfb commented Feb 1, 2016

jimfb commented Feb 1, 2016

sophiebits commented Feb 1, 2016

jimfb commented Feb 24, 2016

mdolbin commented Feb 24, 2016

mdolbin commented Mar 7, 2016

jimfb commented May 3, 2016

sophiebits commented May 3, 2016

gaearon commented Jun 26, 2016

evenstensberg commented Jun 26, 2016

gaearon commented Jun 26, 2016

rricard commented Jun 27, 2016 •

edited

Loading

evenstensberg commented Jun 27, 2016

rricard commented Jun 27, 2016

evenstensberg commented Jun 27, 2016

#5473 warn and ignore setState and forceUpdate calls if rendered on server #5879

#5473 warn and ignore setState and forceUpdate calls if rendered on server #5879

Conversation

mdolbin commented Jan 19, 2016

jimfb commented Jan 19, 2016

facebook-github-bot commented Jan 20, 2016

mdolbin commented Jan 20, 2016

jimfb commented Jan 20, 2016

jimfb Jan 20, 2016

Choose a reason for hiding this comment

mdolbin Jan 20, 2016

Choose a reason for hiding this comment

jimfb Jan 20, 2016

Choose a reason for hiding this comment

mdolbin Jan 20, 2016

Choose a reason for hiding this comment

jimfb commented Jan 20, 2016

sophiebits commented Jan 20, 2016

mdolbin commented Jan 20, 2016

facebook-github-bot commented Jan 21, 2016

facebook-github-bot commented Jan 22, 2016

mdolbin commented Jan 31, 2016

jimfb commented Feb 1, 2016

jimfb commented Feb 1, 2016

sophiebits commented Feb 1, 2016

jimfb commented Feb 24, 2016

mdolbin commented Feb 24, 2016

mdolbin commented Mar 7, 2016

jimfb commented May 3, 2016

sophiebits commented May 3, 2016

gaearon commented Jun 26, 2016

evenstensberg commented Jun 26, 2016

gaearon commented Jun 26, 2016

rricard commented Jun 27, 2016 • edited Loading

evenstensberg commented Jun 27, 2016

rricard commented Jun 27, 2016

evenstensberg commented Jun 27, 2016

rricard commented Jun 27, 2016 •

edited

Loading