Skip to content

Commit

Permalink
Trigger a proper no-op warning for async state changes on server (#7127)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
rricard authored and gaearon committed Jul 4, 2016
1 parent 6e5dd89 commit dbdddf1
Show file tree
Hide file tree
Showing 8 changed files with 260 additions and 15 deletions.
11 changes: 6 additions & 5 deletions src/isomorphic/modern/class/ReactNoopUpdateQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,17 @@

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. ' +
'This usually means you called %s() on an unmounted component. ' +
'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'
);
}
}
Expand Down Expand Up @@ -67,7 +68,7 @@ var ReactNoopUpdateQueue = {
* @internal
*/
enqueueForceUpdate: function(publicInstance) {
warnTDZ(publicInstance, 'forceUpdate');
warnNoop(publicInstance, 'forceUpdate');
},

/**
Expand All @@ -82,7 +83,7 @@ var ReactNoopUpdateQueue = {
* @internal
*/
enqueueReplaceState: function(publicInstance, completeState) {
warnTDZ(publicInstance, 'replaceState');
warnNoop(publicInstance, 'replaceState');
},

/**
Expand All @@ -96,7 +97,7 @@ var ReactNoopUpdateQueue = {
* @internal
*/
enqueueSetState: function(publicInstance, partialState) {
warnTDZ(publicInstance, 'setState');
warnNoop(publicInstance, 'setState');
},
};

Expand Down
10 changes: 9 additions & 1 deletion src/renderers/dom/client/ReactReconcileTransaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ var PooledClass = require('PooledClass');
var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
var ReactInputSelection = require('ReactInputSelection');
var Transaction = require('Transaction');
var ReactUpdateQueue = require('ReactUpdateQueue');


/**
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
9 changes: 9 additions & 0 deletions src/renderers/dom/server/ReactServerRenderingTransaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

var PooledClass = require('PooledClass');
var Transaction = require('Transaction');
var ReactServerUpdateQueue = require('ReactServerUpdateQueue');


/**
Expand All @@ -34,6 +35,7 @@ function ReactServerRenderingTransaction(renderToStaticMarkup) {
this.reinitializeTransaction();
this.renderToStaticMarkup = renderToStaticMarkup;
this.useCreateElement = false;
this.updateQueue = new ReactServerUpdateQueue(this);
}

var Mixin = {
Expand All @@ -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.
Expand Down
132 changes: 132 additions & 0 deletions src/renderers/dom/server/ReactServerUpdateQueue.js
Original file line number Diff line number Diff line change
@@ -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<any, any, any>, 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<any, any, any>): 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<any, any, any>, 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<any, any, any>) {
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<any, any, any>, 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<any, any, any>, partialState: Object|Function) {
if (this.transaction.isInTransaction()) {
ReactUpdateQueue.enqueueSetState(publicInstance, partialState);
} else {
warnNoop(publicInstance, 'setState');
}
}
}

module.exports = ReactServerUpdateQueue;
78 changes: 78 additions & 0 deletions src/renderers/dom/server/__tests__/ReactServerRendering-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <div onClick={() => {}}>{this.state.text}</div>;
}
}

spyOn(console, 'error');
ReactServerRendering.renderToString(<Foo />);
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(<Foo />);
expect(markup).toBe('<div>hello</div>');
});

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 <div onClick={() => {}}>{this.state.text}</div>;
},
});

spyOn(console, 'error');
ReactServerRendering.renderToString(<Bar />);
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(<Bar />);
expect(markup).toBe('<div>hello</div>');
});

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 <div onClick={() => {}}></div>;
},
});

spyOn(console, 'error');
ReactServerRendering.renderToString(<Baz />);
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(<Baz />);
expect(markup).toBe('<div></div>');
});
});
8 changes: 8 additions & 0 deletions src/renderers/native/ReactNativeReconcileTransaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
Loading

0 comments on commit dbdddf1

Please sign in to comment.