From dbdddf1c82884339cdef56326b336227ea51457d Mon Sep 17 00:00:00 2001 From: Robin Ricard Date: Mon, 4 Jul 2016 16:47:00 +0200 Subject: [PATCH] Trigger a proper no-op warning for async state changes on server (#7127) 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 --- .../modern/class/ReactNoopUpdateQueue.js | 11 +- .../dom/client/ReactReconcileTransaction.js | 10 +- .../server/ReactServerRenderingTransaction.js | 9 ++ .../dom/server/ReactServerUpdateQueue.js | 132 ++++++++++++++++++ .../__tests__/ReactServerRendering-test.js | 78 +++++++++++ .../native/ReactNativeReconcileTransaction.js | 8 ++ .../reconciler/ReactCompositeComponent.js | 19 +-- .../testing/ReactTestReconcileTransaction.js | 8 ++ 8 files changed, 260 insertions(+), 15 deletions(-) create mode 100644 src/renderers/dom/server/ReactServerUpdateQueue.js diff --git a/src/isomorphic/modern/class/ReactNoopUpdateQueue.js b/src/isomorphic/modern/class/ReactNoopUpdateQueue.js index 2f0be661c9b5b..49d4c72660a6d 100644 --- a/src/isomorphic/modern/class/ReactNoopUpdateQueue.js +++ b/src/isomorphic/modern/class/ReactNoopUpdateQueue.js @@ -13,8 +13,9 @@ var warning = require('warning'); -function warnTDZ(publicInstance, callerName) { +function warnNoop(publicInstance, callerName) { if (__DEV__) { + var constructor = publicInstance.constructor; warning( false, '%s(...): Can only update a mounted or mounting component. ' + @@ -22,7 +23,7 @@ function warnTDZ(publicInstance, callerName) { 'This is a no-op. Please check the code for the %s component.', callerName, callerName, - publicInstance.constructor && publicInstance.constructor.displayName || '' + constructor && (constructor.displayName || constructor.name) || 'ReactClass' ); } } @@ -67,7 +68,7 @@ var ReactNoopUpdateQueue = { * @internal */ enqueueForceUpdate: function(publicInstance) { - warnTDZ(publicInstance, 'forceUpdate'); + warnNoop(publicInstance, 'forceUpdate'); }, /** @@ -82,7 +83,7 @@ var ReactNoopUpdateQueue = { * @internal */ enqueueReplaceState: function(publicInstance, completeState) { - warnTDZ(publicInstance, 'replaceState'); + warnNoop(publicInstance, 'replaceState'); }, /** @@ -96,7 +97,7 @@ var ReactNoopUpdateQueue = { * @internal */ enqueueSetState: function(publicInstance, partialState) { - warnTDZ(publicInstance, 'setState'); + warnNoop(publicInstance, 'setState'); }, }; diff --git a/src/renderers/dom/client/ReactReconcileTransaction.js b/src/renderers/dom/client/ReactReconcileTransaction.js index 403aa7f2ce8db..bda1f93dc5c34 100644 --- a/src/renderers/dom/client/ReactReconcileTransaction.js +++ b/src/renderers/dom/client/ReactReconcileTransaction.js @@ -16,6 +16,7 @@ var PooledClass = require('PooledClass'); var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter'); var ReactInputSelection = require('ReactInputSelection'); var Transaction = require('Transaction'); +var ReactUpdateQueue = require('ReactUpdateQueue'); /** @@ -104,7 +105,7 @@ var TRANSACTION_WRAPPERS = [ * * @class ReactReconcileTransaction */ -function ReactReconcileTransaction(useCreateElement) { +function ReactReconcileTransaction(useCreateElement: boolean) { this.reinitializeTransaction(); // Only server-side rendering really needs this option (see // `ReactServerRendering`), but server-side uses @@ -135,6 +136,13 @@ var Mixin = { return this.reactMountReady; }, + /** + * @return {object} The queue to collect React async events. + */ + getUpdateQueue: function() { + return ReactUpdateQueue; + }, + /** * Save current transaction state -- if the return value from this method is * passed to `rollback`, the transaction will be reset to that state. diff --git a/src/renderers/dom/server/ReactServerRenderingTransaction.js b/src/renderers/dom/server/ReactServerRenderingTransaction.js index e7909eeeedf13..9e559ef08c359 100644 --- a/src/renderers/dom/server/ReactServerRenderingTransaction.js +++ b/src/renderers/dom/server/ReactServerRenderingTransaction.js @@ -13,6 +13,7 @@ var PooledClass = require('PooledClass'); var Transaction = require('Transaction'); +var ReactServerUpdateQueue = require('ReactServerUpdateQueue'); /** @@ -34,6 +35,7 @@ function ReactServerRenderingTransaction(renderToStaticMarkup) { this.reinitializeTransaction(); this.renderToStaticMarkup = renderToStaticMarkup; this.useCreateElement = false; + this.updateQueue = new ReactServerUpdateQueue(this); } var Mixin = { @@ -54,6 +56,13 @@ var Mixin = { return noopCallbackQueue; }, + /** + * @return {object} The queue to collect React async events. + */ + getUpdateQueue: function() { + return this.updateQueue; + }, + /** * `PooledClass` looks for this, and will invoke this before allowing this * instance to be reused. diff --git a/src/renderers/dom/server/ReactServerUpdateQueue.js b/src/renderers/dom/server/ReactServerUpdateQueue.js new file mode 100644 index 0000000000000..4033339055bbc --- /dev/null +++ b/src/renderers/dom/server/ReactServerUpdateQueue.js @@ -0,0 +1,132 @@ +/** + * Copyright 2015-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + * @providesModule ReactServerUpdateQueue + * @flow + */ + +'use strict'; + +var ReactUpdateQueue = require('ReactUpdateQueue'); +var Transaction = require('Transaction'); +var warning = require('warning'); + +function warnNoop(publicInstance: ReactComponent, callerName: string) { + if (__DEV__) { + var constructor = publicInstance.constructor; + warning( + false, + '%s(...): Can only update a mounting component. ' + + 'This usually means you called %s() outside componentWillMount() on the server. ' + + 'This is a no-op. Please check the code for the %s component.', + callerName, + callerName, + constructor && (constructor.displayName || constructor.name) || 'ReactClass' + ); + } +} + +/** + * This is the update queue used for server rendering. + * It delegates to ReactUpdateQueue while server rendering is in progress and + * switches to ReactNoopUpdateQueue after the transaction has completed. + * @class ReactServerUpdateQueue + * @param {Transaction} transaction + */ +class ReactServerUpdateQueue { + /* :: transaction: Transaction; */ + + constructor(transaction: Transaction) { + this.transaction = transaction; + } + + /** + * Checks whether or not this composite component is mounted. + * @param {ReactClass} publicInstance The instance we want to test. + * @return {boolean} True if mounted, false otherwise. + * @protected + * @final + */ + isMounted(publicInstance: ReactComponent): boolean { + return false; + } + + /** + * Enqueue a callback that will be executed after all the pending updates + * have processed. + * + * @param {ReactClass} publicInstance The instance to use as `this` context. + * @param {?function} callback Called after state is updated. + * @internal + */ + enqueueCallback(publicInstance: ReactComponent, callback?: Function, callerName?: string) { + if (this.transaction.isInTransaction()) { + ReactUpdateQueue.enqueueCallback(publicInstance, callback, callerName); + } + } + + /** + * Forces an update. This should only be invoked when it is known with + * certainty that we are **not** in a DOM transaction. + * + * You may want to call this when you know that some deeper aspect of the + * component's state has changed but `setState` was not called. + * + * This will not invoke `shouldComponentUpdate`, but it will invoke + * `componentWillUpdate` and `componentDidUpdate`. + * + * @param {ReactClass} publicInstance The instance that should rerender. + * @internal + */ + enqueueForceUpdate(publicInstance: ReactComponent) { + if (this.transaction.isInTransaction()) { + ReactUpdateQueue.enqueueForceUpdate(publicInstance); + } else { + warnNoop(publicInstance, 'forceUpdate'); + } + } + + /** + * Replaces all of the state. Always use this or `setState` to mutate state. + * You should treat `this.state` as immutable. + * + * There is no guarantee that `this.state` will be immediately updated, so + * accessing `this.state` after calling this method may return the old value. + * + * @param {ReactClass} publicInstance The instance that should rerender. + * @param {object|function} completeState Next state. + * @internal + */ + enqueueReplaceState(publicInstance: ReactComponent, completeState: Object|Function) { + if (this.transaction.isInTransaction()) { + ReactUpdateQueue.enqueueReplaceState(publicInstance, completeState); + } else { + warnNoop(publicInstance, 'replaceState'); + } + } + + /** + * Sets a subset of the state. This only exists because _pendingState is + * internal. This provides a merging strategy that is not available to deep + * properties which is confusing. TODO: Expose pendingState or don't use it + * during the merge. + * + * @param {ReactClass} publicInstance The instance that should rerender. + * @param {object|function} partialState Next partial state to be merged with state. + * @internal + */ + enqueueSetState(publicInstance: ReactComponent, partialState: Object|Function) { + if (this.transaction.isInTransaction()) { + ReactUpdateQueue.enqueueSetState(publicInstance, partialState); + } else { + warnNoop(publicInstance, 'setState'); + } + } +} + +module.exports = ReactServerUpdateQueue; diff --git a/src/renderers/dom/server/__tests__/ReactServerRendering-test.js b/src/renderers/dom/server/__tests__/ReactServerRendering-test.js index 0ebabbfd67ae2..8ee8f18551212 100644 --- a/src/renderers/dom/server/__tests__/ReactServerRendering-test.js +++ b/src/renderers/dom/server/__tests__/ReactServerRendering-test.js @@ -400,4 +400,82 @@ describe('ReactServerRendering', function() { expect(markup.indexOf('hello, world') >= 0).toBe(true); }); }); + + it('warns with a no-op when an async setState is triggered', function() { + class Foo extends React.Component { + componentWillMount() { + this.setState({text: 'hello'}); + setTimeout(() => { + this.setState({text: 'error'}); + }); + } + render() { + return
{}}>{this.state.text}
; + } + } + + spyOn(console, 'error'); + ReactServerRendering.renderToString(); + jest.runOnlyPendingTimers(); + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.mostRecent().args[0]).toBe( + 'Warning: setState(...): Can only update a mounting component.' + + ' This usually means you called setState() outside componentWillMount() on the server.' + + ' This is a no-op. Please check the code for the Foo component.' + ); + var markup = ReactServerRendering.renderToStaticMarkup(); + expect(markup).toBe('
hello
'); + }); + + it('warns with a no-op when an async replaceState is triggered', function() { + var Bar = React.createClass({ + componentWillMount: function() { + this.replaceState({text: 'hello'}); + setTimeout(() => { + this.replaceState({text: 'error'}); + }); + }, + render: function() { + return
{}}>{this.state.text}
; + }, + }); + + spyOn(console, 'error'); + ReactServerRendering.renderToString(); + jest.runOnlyPendingTimers(); + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.mostRecent().args[0]).toBe( + 'Warning: replaceState(...): Can only update a mounting component. ' + + 'This usually means you called replaceState() outside componentWillMount() on the server. ' + + 'This is a no-op. Please check the code for the Bar component.' + ); + var markup = ReactServerRendering.renderToStaticMarkup(); + expect(markup).toBe('
hello
'); + }); + + it('warns with a no-op when an async forceUpdate is triggered', function() { + var Baz = React.createClass({ + componentWillMount: function() { + this.forceUpdate(); + setTimeout(() => { + this.forceUpdate(); + }); + }, + render: function() { + return
{}}>
; + }, + }); + + spyOn(console, 'error'); + ReactServerRendering.renderToString(); + jest.runOnlyPendingTimers(); + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.mostRecent().args[0]).toBe( + 'Warning: forceUpdate(...): Can only update a mounting component. ' + + 'This usually means you called forceUpdate() outside componentWillMount() on the server. ' + + 'This is a no-op. Please check the code for the Baz component.' + ); + var markup = ReactServerRendering.renderToStaticMarkup(); + expect(markup).toBe('
'); + }); }); diff --git a/src/renderers/native/ReactNativeReconcileTransaction.js b/src/renderers/native/ReactNativeReconcileTransaction.js index 1c40be74bc4e2..e2eba61e5e413 100644 --- a/src/renderers/native/ReactNativeReconcileTransaction.js +++ b/src/renderers/native/ReactNativeReconcileTransaction.js @@ -14,6 +14,7 @@ var CallbackQueue = require('CallbackQueue'); var PooledClass = require('PooledClass'); var Transaction = require('Transaction'); +var ReactUpdateQueue = require('ReactUpdateQueue'); /** * Provides a `CallbackQueue` queue for collecting `onDOMReady` callbacks during @@ -81,6 +82,13 @@ var Mixin = { return this.reactMountReady; }, + /** + * @return {object} The queue to collect React async events. + */ + getUpdateQueue: function() { + return ReactUpdateQueue; + }, + /** * `PooledClass` looks for this, and will invoke this before allowing this * instance to be reused. diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index d12995062a724..142598ecd4361 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -20,7 +20,6 @@ var ReactInstrumentation = require('ReactInstrumentation'); var ReactNodeTypes = require('ReactNodeTypes'); var ReactPropTypeLocations = require('ReactPropTypeLocations'); var ReactReconciler = require('ReactReconciler'); -var ReactUpdateQueue = require('ReactUpdateQueue'); var checkReactTypeSpec = require('checkReactTypeSpec'); @@ -197,8 +196,10 @@ var ReactCompositeComponentMixin = { var Component = this._currentElement.type; + var updateQueue = transaction.getUpdateQueue(); + // Initialize the public class - var inst = this._constructComponent(publicProps, publicContext); + var inst = this._constructComponent(publicProps, publicContext, updateQueue); var renderedElement; // Support functional components @@ -245,7 +246,7 @@ var ReactCompositeComponentMixin = { inst.props = publicProps; inst.context = publicContext; inst.refs = emptyObject; - inst.updater = ReactUpdateQueue; + inst.updater = updateQueue; this._instance = inst; @@ -352,20 +353,20 @@ var ReactCompositeComponentMixin = { return markup; }, - _constructComponent: function(publicProps, publicContext) { + _constructComponent: function(publicProps, publicContext, updateQueue) { if (__DEV__) { ReactCurrentOwner.current = this; try { - return this._constructComponentWithoutOwner(publicProps, publicContext); + return this._constructComponentWithoutOwner(publicProps, publicContext, updateQueue); } finally { ReactCurrentOwner.current = null; } } else { - return this._constructComponentWithoutOwner(publicProps, publicContext); + return this._constructComponentWithoutOwner(publicProps, publicContext, updateQueue); } }, - _constructComponentWithoutOwner: function(publicProps, publicContext) { + _constructComponentWithoutOwner: function(publicProps, publicContext, updateQueue) { var Component = this._currentElement.type; var instanceOrElement; if (shouldConstruct(Component)) { @@ -377,7 +378,7 @@ var ReactCompositeComponentMixin = { ); } } - instanceOrElement = new Component(publicProps, publicContext, ReactUpdateQueue); + instanceOrElement = new Component(publicProps, publicContext, updateQueue); if (__DEV__) { if (this._debugID !== 0) { ReactInstrumentation.debugTool.onEndLifeCycleTimer( @@ -397,7 +398,7 @@ var ReactCompositeComponentMixin = { ); } } - instanceOrElement = Component(publicProps, publicContext, ReactUpdateQueue); + instanceOrElement = Component(publicProps, publicContext, updateQueue); if (__DEV__) { if (this._debugID !== 0) { ReactInstrumentation.debugTool.onEndLifeCycleTimer( diff --git a/src/renderers/testing/ReactTestReconcileTransaction.js b/src/renderers/testing/ReactTestReconcileTransaction.js index 94716e59d8608..e0b959784eaf1 100644 --- a/src/renderers/testing/ReactTestReconcileTransaction.js +++ b/src/renderers/testing/ReactTestReconcileTransaction.js @@ -14,6 +14,7 @@ var CallbackQueue = require('CallbackQueue'); var PooledClass = require('PooledClass'); var Transaction = require('Transaction'); +var ReactUpdateQueue = require('ReactUpdateQueue'); /** * Provides a `CallbackQueue` queue for collecting `onDOMReady` callbacks during @@ -81,6 +82,13 @@ var Mixin = { return this.reactMountReady; }, + /** + * @return {object} The queue to collect React async events. + */ + getUpdateQueue: function() { + return ReactUpdateQueue; + }, + /** * `PooledClass` looks for this, and will invoke this before allowing this * instance to be reused.