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

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

Merged
merged 1 commit into from
Jul 4, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Collaborator

@gaearon gaearon Jun 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

 * 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.

* @param {Transaction} transaction
*/
class ReactServerUpdateQueue {
/* :: transaction: Transaction; */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don’t use this convention anywhere so let’s remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually needed by flow to be able to assign this.transaction.

Copy link
Contributor Author

@rricard rricard Jun 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without comments requires two special babel plugins (syntax-flow & transform-flow-strip-types) I haven't checked if it there are there yet...

Copy link
Contributor Author

@rricard rricard Jun 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch that, the transform needed there is an ES7 transform: syntax-class-properties


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()) {
Copy link
Collaborator

@gaearon gaearon Jun 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always warn it’s a no-op here, forceUpdate() doesn’t make sense on the server.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, scratch that, maybe your render is impure and something just changed. Let’s leave this as is.

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we just write this as ES class? We use Babel now so I don’t see why not just use the class syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! Cool! I thought that this part wasn't quite yet babel-ready. I should have checked the gulp pipeline ...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I’m not 100% sure (we don’t seem to use classes yet) but this could be the first one 😉 . I think classes is in babelrc so it should be good. But check compiled output to make sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course! I can change the babelrc if needed...

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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon any particular reason to use spyOn instead of jest.fn()? I know @cpojer has recommended using jest.fn over spyOn to me in the past.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don’t know. That’s what the rest of the codebase does tho.

Copy link
Member

@zpao zpao Jun 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historically we ran these tests in regular phantom in the browser, before jest really existed. We did that for a while before turning off last year. We wanted to maintain that option value moving forward and tying too closely to jest would make it harder (unless jest starts supporting that use case).

Copy link
Contributor Author

@rricard rricard Jun 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I'll stick with the jasmine-way of spying on things...

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