Trigger a proper no-op warning for async state changes on server #7127

rricard · 2016-06-27T08:55:30Z

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 ReactNoopUpdateQueue's methods afterwards
Added a test that mimics the case presented in ReactDOMServer.renderToString: presence of onClick handler causes errors on async update #5473 with setState and
replaceState

rricard · 2016-06-27T09:00:42Z

src/renderers/dom/server/ReactServerRenderingTransaction.js

+    }, this)
+    .reduce(function(ReactUpdate, funcObj) {
+      return Object.assign(ReactUpdate, funcObj);
+    });


Not sure about this though, maybe there is a way to do better there. I personally would have used Immutable for this kind of trick, but, I understand the constraints here. So if I can do that differently, just tell me!

Can you please unwind this to be simple (repetitive) code instead? IMO meta programming is not worth it here :-)

Do I understand correctly that the intention is to use ReactUpdateQueue during mounting so initial setState works but switch it out right after the mounting is over?

We would also need a test for forceUpdate.

I can do that, especially if I can consider that UpdateQueues will be identical. Yes, you understood that correctly, should I change my comment to be more clear maybe ?

It will be more obvious after the unwinding.

rricard · 2016-06-27T09:10:21Z

src/renderers/dom/client/ReactReconcileTransaction.js

@@ -7,6 +7,7 @@
 * of patent rights can be found in the PATENTS file in the same directory.
 *
 * @providesModule ReactReconcileTransaction
+ * @flow weak


Oh! maybe you don't want those! Can delete them if you ask, but flow helped me though (once again!)...

Would it be possible to add any type annotations if you're already using flow? I know there have been some recent efforts by @vjeux to add annotations. I don't see any files that are using weak mode either so that might be something to consider.

I can totally do that! I wasn't sure about babel on the React codebase but as @gaearon pointed out, babel is there so, yes I can do it. (After this, I can also start to add some flow throughout the codebase if you are ok with that, you can even point me to critical parts. However, the flow declarations for react already are super complete, so I'm not sure there is really critical parts).

I have no idea what flow weak does, can you educate me? Also what isn't working with normal flow?

There is 53 flow errors in ReactCompositeComponent (missing annotations), I'll do this file in a separate PR.

weak allows unannotated function defs, it is often used for progressive addition of flow, that way you can start typechecking but in the absence of annotation, flow will consider the params automatically as any. https://flowtype.org/docs/existing.html#weak-mode

For instance, I think you could add @flow weak everywhere in this codebase without raising much issues. The upside would be that flow could still emit warnings on very risky things it managed to infer.

rricard · 2016-06-27T12:06:30Z

Still working on it, don't trust the bot ! I will rebase/squash at the end don't worry !

rricard · 2016-06-27T12:31:49Z

@gaearon: took your comments into consideration and rebased/squashed the changes.

gaearon · 2016-06-27T13:35:06Z

src/renderers/dom/server/ReactServerRenderingTransaction.js

+    // We use a ReactNoopUpdateQueue that switches to ReactUpdateQueue while
+    // mounting
+    var transaction = this;
+    return {


This allocates an object any time getUpdateQueue is called. Instead, it should only be initialized once. For example, you could write a class called ReactServerUpdateQueue that would take transaction as a constructor argument.

Yes, good catch! I will refactor that!

rricard · 2016-06-27T14:24:13Z

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

+      'Warning: setState(...): Can only update a mounted or mounting' +
+      ' component. This usually means you called setState() on an' +
+      ' unmounted component. This is a no-op. Please check the code for' +
+      ' the Component component.'


I would actually change the message to something more adapted to the server:
Warning: setState(...): Can only update a mounting component. This usually means you called setState() on an unmounted component or already mounted component on a server. This is a no-op. Please check the code for the Component component.

I agree (if it’s not super difficult). Note that server has no concept of “already mounted component”. I would say “This usually means you called setState() outside componentWillMount() on the server.” or something like this.

I'll think of something. I really like react warnings because they are super-clear, I want to continue in this direction here...

rricard · 2016-06-27T16:44:24Z

Finishing with this and squashing/rebasing then

rricard · 2016-06-27T16:44:54Z

Again, don't trust the bot, still needs revision!

gaearon · 2016-06-30T15:39:36Z

Thanks!

rricard · 2016-06-30T15:40:58Z

No problem, I was going to do a second PR for that afterwards but I can do that just now!

rricard · 2016-06-30T16:30:02Z

Ok, I will need some time to do that ... I need to go deep sometimes...

rricard · 2016-06-30T18:34:07Z

Ok so I'll just remove some annotations here so we can merge it. Still, I'll stash my work to continue that afterwards!

rricard · 2016-06-30T19:01:09Z

@gaearon so I only left the flow typecheck in ReactServerUpdateQueue. I think it's mergeable now. I'll continue with flow in other PRs!

gaearon · 2016-06-30T19:06:24Z

src/renderers/dom/server/ReactServerRenderingTransaction.js

@@ -30,10 +31,11 @@ var noopCallbackQueue = {
 * @class ReactServerRenderingTransaction
 * @param {boolean} renderToStaticMarkup
 */
-function ReactServerRenderingTransaction(renderToStaticMarkup) {
+function ReactServerRenderingTransaction(renderToStaticMarkup: boolean) {


Nit: : boolean is now unnecessary here.

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

gaearon · 2016-07-04T14:47:05Z

Thanks!

It was added in #7127 but this file isn’t type checked anyway.

rricard · 2016-07-04T15:01:48Z

Thank you for reviewing me. That was a good and instructive experience!

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)

…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

It was added in facebook#7127 but this file isn’t type checked anyway.

graue · 2016-11-22T23:06:36Z

Looks like this was actually a breaking change. It broke some previously working setState calls.

gaearon · 2016-11-22T23:26:56Z

Can you file an issue and provide code reproducing it? setState calls while rendering is in progress should still work. After mounting is over they should become no-ops instead of throwing like before. What kind of calls are breaking?

ghost added GH Review: review-needed CLA Signed labels Jun 27, 2016

rricard reviewed Jun 27, 2016
View reviewed changes

rricard mentioned this pull request Jun 27, 2016

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

Closed

rricard reviewed Jun 27, 2016
View reviewed changes

gaearon added GH Review: needs-revision and removed GH Review: review-needed labels Jun 27, 2016

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Jun 27, 2016

rricard changed the title ~~Trigger a proper no-op warning on async setState on server~~ Trigger a proper no-op warning for async state changes on server Jun 27, 2016

gaearon reviewed Jun 27, 2016
View reviewed changes

gaearon added GH Review: needs-revision and removed GH Review: review-needed labels Jun 27, 2016

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Jun 27, 2016

rricard reviewed Jun 27, 2016
View reviewed changes

gaearon added GH Review: needs-revision and removed GH Review: review-needed labels Jun 27, 2016

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Jun 27, 2016

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Jun 30, 2016

gaearon reviewed Jun 30, 2016
View reviewed changes

gaearon added GH Review: accepted and removed GH Review: review-needed labels Jul 4, 2016

gaearon added this to the 15-next milestone Jul 4, 2016

gaearon merged commit dbdddf1 into facebook:master Jul 4, 2016

gaearon added a commit that referenced this pull request Jul 4, 2016

Remove unnecessary Flow annotation

24dfb56

It was added in #7127 but this file isn’t type checked anyway.

rricard deleted the fix-5473 branch July 4, 2016 15:01

zpao modified the milestones: 15-next, 15.2.1 Jul 8, 2016

usmanajmal pushed a commit to usmanajmal/react that referenced this pull request Jul 11, 2016

Remove unnecessary Flow annotation

4a38037

It was added in facebook#7127 but this file isn’t type checked anyway.

keyz mentioned this pull request Jul 21, 2016

Improve warning in ReactUpdateQueue #7326

Merged

renovate bot mentioned this pull request Feb 2, 2018

Update dependency react to v0.14.9 - autoclosed signavio/backbone-rel#29

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trigger a proper no-op warning for async state changes on server #7127

Trigger a proper no-op warning for async state changes on server #7127

rricard commented Jun 27, 2016 •

edited

Loading

rricard Jun 27, 2016

gaearon Jun 27, 2016

gaearon Jun 27, 2016 •

edited

Loading

gaearon Jun 27, 2016

rricard Jun 27, 2016 •

edited

Loading

gaearon Jun 27, 2016

rricard Jun 27, 2016

aweary Jun 27, 2016

rricard Jun 27, 2016

vjeux Jun 27, 2016

rricard Jun 27, 2016

rricard Jun 27, 2016 •

edited

Loading

rricard Jun 27, 2016

rricard commented Jun 27, 2016 •

edited

Loading

rricard commented Jun 27, 2016

gaearon Jun 27, 2016

rricard Jun 27, 2016

rricard Jun 27, 2016

gaearon Jun 27, 2016

rricard Jun 27, 2016 •

edited

Loading

rricard commented Jun 27, 2016

rricard commented Jun 27, 2016

gaearon commented Jun 30, 2016

rricard commented Jun 30, 2016

rricard commented Jun 30, 2016

rricard commented Jun 30, 2016

rricard commented Jun 30, 2016

gaearon Jun 30, 2016

rricard Jun 30, 2016

gaearon commented Jul 4, 2016

rricard commented Jul 4, 2016

graue commented Nov 22, 2016

gaearon commented Nov 22, 2016

Trigger a proper no-op warning for async state changes on server #7127

Trigger a proper no-op warning for async state changes on server #7127

Conversation

rricard commented Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rricard Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rricard Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rricard commented Jun 27, 2016 • edited Loading

rricard commented Jun 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rricard Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

rricard commented Jun 27, 2016

rricard commented Jun 27, 2016

gaearon commented Jun 30, 2016

rricard commented Jun 30, 2016

rricard commented Jun 30, 2016

rricard commented Jun 30, 2016

rricard commented Jun 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Jul 4, 2016

rricard commented Jul 4, 2016

graue commented Nov 22, 2016

gaearon commented Nov 22, 2016

rricard commented Jun 27, 2016 •

edited

Loading

gaearon Jun 27, 2016 •

edited

Loading

rricard Jun 27, 2016 •

edited

Loading

rricard Jun 27, 2016 •

edited

Loading

rricard commented Jun 27, 2016 •

edited

Loading

rricard Jun 27, 2016 •

edited

Loading