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.

Sign up for GitHub

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

[Fiber] Complete ES6 Class related errors support #8156

Merged
merged 15 commits into from
Nov 5, 2016
Merged

[Fiber] Complete ES6 Class related errors support #8156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
306518c
Print a warning in fiber for lacking render method
rricard Oct 30, 2016
071b884
Reject initial non-object state in Fiber
rricard Oct 30, 2016
9618961
Centralize fiber checks on ES6 Classes
rricard Oct 30, 2016
184a82a
Add classic & instance property checks on fiber
rricard Oct 30, 2016
4a87efd
Convert to normalized React `warning` calls
rricard Oct 30, 2016
edb58c2
Support lifecycle typo detection in fiber
rricard Oct 30, 2016
3c3f44b
Get the complete warnings from the existing code
rricard Oct 30, 2016
a69001c
Only check classInstance once
rricard Oct 30, 2016
102ece5
Can warn component while resuming but only once
rricard Oct 30, 2016
2b0fc93
Remove warning deduplication
rricard Oct 31, 2016
89377af
Factor name retrieval in a getName helper
rricard Oct 31, 2016
6a92c3f
Use invariant instead of throw
rricard Oct 31, 2016
f8289a7
Read inst.state only once for the invariant
rricard Oct 31, 2016
9ae84f1
Fix condition on the instance state
rricard Nov 1, 2016
c8dc3c9
Register failing/passing fiber tests
rricard Nov 2, 2016
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
16 changes: 0 additions & 16 deletions scripts/fiber/tests-failing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,30 +48,15 @@ src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js
* includes the owner name when passing null, undefined, boolean, or number

src/isomorphic/modern/class/__tests__/ReactCoffeeScriptClass-test.coffee
* throws if no render function is defined
* renders based on context in the constructor
* should throw with non-object in the initial state property
* warns when classic properties are defined on the instance, but does not invoke them.
* should warn when misspelling shouldComponentUpdate
* should warn when misspelling componentWillReceiveProps
* supports this.context passed via getChildContext

src/isomorphic/modern/class/__tests__/ReactES6Class-test.js
* throws if no render function is defined
* renders based on context in the constructor
* should throw with non-object in the initial state property
* warns when classic properties are defined on the instance, but does not invoke them.
* should warn when misspelling shouldComponentUpdate
* should warn when misspelling componentWillReceiveProps
* supports this.context passed via getChildContext

src/isomorphic/modern/class/__tests__/ReactTypeScriptClass-test.ts
* throws if no render function is defined
* renders based on context in the constructor
* should throw with non-object in the initial state property
* warns when classic properties are defined on the instance, but does not invoke them.
* should warn when misspelling shouldComponentUpdate
* should warn when misspelling componentWillReceiveProps
* supports this.context passed via getChildContext

src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js
Expand Down Expand Up @@ -538,7 +523,6 @@ src/renderers/shared/stack/reconciler/__tests__/ReactCompositeComponent-test.js
* should warn about `setState` in render
* should warn about `setState` in getChildContext
* should warn when shouldComponentUpdate() returns undefined
* should warn when componentDidUnmount method is defined
* should pass context to children when not owner
* should pass context when re-rendered for static child
* should pass context when re-rendered for static child within a composite component
Expand Down
16 changes: 16 additions & 0 deletions scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,30 +350,40 @@ src/isomorphic/modern/class/__tests__/ReactClassEquivalence-test.js

src/isomorphic/modern/class/__tests__/ReactCoffeeScriptClass-test.coffee
* preserves the name of the class for use in error messages
* throws if no render function is defined
* renders a simple stateless component with prop
* renders based on state using initial values in this.props
* renders based on state using props in the constructor
* renders only once when setting state in componentWillMount
* should throw with non-object in the initial state property
* should render with null in the initial state property
* setState through an event handler
* should not implicitly bind event handlers
* renders using forceUpdate even when there is no state
* will call all the normal life cycle methods
* warns when classic properties are defined on the instance, but does not invoke them.
* should warn when misspelling shouldComponentUpdate
* should warn when misspelling componentWillReceiveProps
* should throw AND warn when trying to access classic APIs
* supports classic refs
* supports drilling through to the DOM using findDOMNode

src/isomorphic/modern/class/__tests__/ReactES6Class-test.js
* preserves the name of the class for use in error messages
* throws if no render function is defined
* renders a simple stateless component with prop
* renders based on state using initial values in this.props
* renders based on state using props in the constructor
* renders only once when setting state in componentWillMount
* should throw with non-object in the initial state property
* should render with null in the initial state property
* setState through an event handler
* should not implicitly bind event handlers
* renders using forceUpdate even when there is no state
* will call all the normal life cycle methods
* warns when classic properties are defined on the instance, but does not invoke them.
* should warn when misspelling shouldComponentUpdate
* should warn when misspelling componentWillReceiveProps
* should throw AND warn when trying to access classic APIs
* supports classic refs
* supports drilling through to the DOM using findDOMNode
Expand All @@ -385,15 +395,20 @@ src/isomorphic/modern/class/__tests__/ReactPureComponent-test.js

src/isomorphic/modern/class/__tests__/ReactTypeScriptClass-test.ts
* preserves the name of the class for use in error messages
* throws if no render function is defined
* renders a simple stateless component with prop
* renders based on state using initial values in this.props
* renders based on state using props in the constructor
* renders only once when setting state in componentWillMount
* should throw with non-object in the initial state property
* should render with null in the initial state property
* setState through an event handler
* should not implicitly bind event handlers
* renders using forceUpdate even when there is no state
* will call all the normal life cycle methods
* warns when classic properties are defined on the instance, but does not invoke them.
* should warn when misspelling shouldComponentUpdate
* should warn when misspelling componentWillReceiveProps
* should throw AND warn when trying to access classic APIs
* supports classic refs
* supports drilling through to the DOM using findDOMNode
Expand Down Expand Up @@ -1002,6 +1017,7 @@ src/renderers/shared/stack/reconciler/__tests__/ReactCompositeComponent-test.js
* should silently allow `setState`, not call cb on unmounting components
* should cleanup even if render() fatals
* should call componentWillUnmount before unmounting
* should warn when componentDidUnmount method is defined
* should skip update when rerendering element in container
* only renders once if updated in componentWillReceiveProps
* should allow access to findDOMNode in componentWillUnmount
Expand Down
98 changes: 98 additions & 0 deletions src/renderers/shared/fiber/ReactFiberClassComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ var {
var { isMounted } = require('ReactFiberTreeReflection');
var ReactInstanceMap = require('ReactInstanceMap');
var shallowEqual = require('shallowEqual');
var warning = require('warning');
var invariant = require('invariant');


const isArray = Array.isArray;

module.exports = function(scheduleUpdate : (fiber: Fiber) => void) {

Expand Down Expand Up @@ -91,6 +96,98 @@ module.exports = function(scheduleUpdate : (fiber: Fiber) => void) {
return true;
}

function getName(workInProgress: Fiber, inst: any): string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a module that lets you do this now. getComponentName.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess that module doesn't use inst.constructor as a fallback. Hm. Maybe it should? Don't know. I'll land this for now while we figure that out.

const type = workInProgress.type;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please not calculate these fields in production if we're not planning to print anything?

It's fine to extract this to getFriendlyName(inst) and only call this in __DEV__ branch and in both-dev-and-prod if () branch below when we're sure we'll print the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'm going to change that

const constructor = inst && inst.constructor;
return (
type.displayName || (constructor && constructor.displayName) ||
type.name || (constructor && constructor.name) ||
'A Component'
);
}

function checkClassInstance(workInProgress: Fiber, inst: any) {
if (__DEV__) {
const name = getName(workInProgress, inst);
const renderPresent = inst.render;
warning(
renderPresent,
'%s(...): No `render` method found on the returned component ' +
'instance: you may have forgotten to define `render`.',
name
);
const noGetInitialStateOnES6 = (
!inst.getInitialState ||
inst.getInitialState.isReactClassApproved
);
warning(
noGetInitialStateOnES6,
'getInitialState was defined on %s, a plain JavaScript class. ' +
'This is only supported for classes created using React.createClass. ' +
'Did you mean to define a state property instead?',
name
);
const noGetDefaultPropsOnES6 = (
!inst.getDefaultProps ||
inst.getDefaultProps.isReactClassApproved
);
warning(
noGetDefaultPropsOnES6,
'getDefaultProps was defined on %s, a plain JavaScript class. ' +
'This is only supported for classes created using React.createClass. ' +
'Use a static property to define defaultProps instead.',
name
);
const noInstancePropTypes = !inst.propTypes;
warning(
noInstancePropTypes,
'propTypes was defined as an instance property on %s. Use a static ' +
'property to define propTypes instead.',
name,
);
const noInstanceContextTypes = !inst.contextTypes;
warning(
noInstanceContextTypes,
'contextTypes was defined as an instance property on %s. Use a static ' +
'property to define contextTypes instead.',
name,
);
const noComponentShouldUpdate = typeof inst.componentShouldUpdate !== 'function';
warning(
noComponentShouldUpdate,
'%s has a method called ' +
'componentShouldUpdate(). Did you mean shouldComponentUpdate()? ' +
'The name is phrased as a question because the function is ' +
'expected to return a value.',
name
);
const noComponentDidUnmount = typeof inst.componentDidUnmount !== 'function';
warning(
noComponentDidUnmount,
'%s has a method called ' +
'componentDidUnmount(). But there is no such lifecycle method. ' +
'Did you mean componentWillUnmount()?',
name
);
const noComponentWillRecieveProps = typeof inst.componentWillRecieveProps !== 'function';
warning(
noComponentWillRecieveProps,
'%s has a method called ' +
'componentWillRecieveProps(). Did you mean componentWillReceiveProps()?',
name
);
}

const instanceState = inst.state;
if (instanceState && (typeof instanceState !== 'object' || isArray(instanceState))) {
invariant(
false,
'%s.state: must be set to an object or null',
getName(workInProgress, inst)
);
}
}

function adoptClassInstance(workInProgress : Fiber, instance : any) : void {
instance.updater = updater;
workInProgress.stateNode = instance;
Expand All @@ -102,6 +199,7 @@ module.exports = function(scheduleUpdate : (fiber: Fiber) => void) {
const ctor = workInProgress.type;
const props = workInProgress.pendingProps;
const instance = new ctor(props);
checkClassInstance(workInProgress, instance);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we avoid checking if we are resuming work and recreating the instance? Check should only be done once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, That's the interesting part! I'll try to do that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't that hard ... Maybe there is a more elegant way to do that though ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, this is not sufficient since the state may depend on props so it can yield different results during the rerender. This would then only throw an error during race conditions.

class Foo extends React.Component {
  state = this.props.bar ? {} : [];
}

I'd rather check it every time.

For warnings, we should probably ideally dedupe though.

adoptClassInstance(workInProgress, instance);
return instance;
}
Expand Down