[Fiber] Complete ES6 Class related errors support #8156

rricard · 2016-10-30T16:10:01Z

Print a warning in fiber for lacking render method
Throw with non-object in the initial state property
Classic & instance property checks
Typos in lifecycle methods

rricard · 2016-10-30T16:13:52Z

Will keep ReactES6Class › renders based on context in the constructor & ReactES6Class › supports this.context passed via getChildContext aside for now, will work later on a dedicated PR for context transmission.

gaearon · 2016-10-30T19:05:01Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -107,6 +177,7 @@ module.exports = function(scheduleUpdate : (fiber: Fiber, priorityLevel : Priori
    const ctor = workInProgress.type;
    const props = workInProgress.pendingProps;
    const instance = new ctor(props);
+    checkClassInstance(workInProgress, instance);


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

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

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

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.

sebmarkbage · 2016-10-30T19:36:00Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

  function adoptClassInstance(workInProgress : Fiber, instance : any) : void {
    instance.updater = updater;
    workInProgress.stateNode = instance;
    // The instance needs access to the fiber so that it can schedule updates
    ReactInstanceMap.set(instance, workInProgress);
  }

-  function constructClassInstance(workInProgress : Fiber) : any {
+  function constructClassInstance(workInProgress : Fiber, resuming?: boolean) : any {


We should not be using optional arguments but I'd rather restructure this to not use runtime flags.

sebmarkbage · 2016-10-30T19:39:59Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -107,6 +177,7 @@ module.exports = function(scheduleUpdate : (fiber: Fiber, priorityLevel : Priori
    const ctor = workInProgress.type;
    const props = workInProgress.pendingProps;
    const instance = new ctor(props);
+    checkClassInstance(workInProgress, instance);


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.

sebmarkbage · 2016-10-30T19:41:42Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

+      );
+    }
+
+    if (inst.state && (typeof inst.state !== 'object' || Array.isArray(inst.state))) {


Can you move isArray to the module scope like this?

react/src/renderers/shared/fiber/ReactChildFiber.js

Line 48 in d5eff3b

const isArray = Array.isArray;

sebmarkbage · 2016-10-30T19:42:32Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

+    }
+
+    if (inst.state && (typeof inst.state !== 'object' || Array.isArray(inst.state))) {
+      throw new Error(`${name}.state: must be set to an object or null`);


Can you add a comment like // TODO: Change this to be a warning.?

I don't think this needs to be an error but might as well do this for compatibility for now.

rricard · 2016-10-30T20:47:51Z

Same on this PR even if there is less work left, I need to take the time to do things correctly (warning dedupe for instance).

rricard · 2016-10-30T22:05:24Z

@sebmarkbage had to add a field in the Fiber struct in order to avoid warning twice. Maybe you have a better idea in order to avoid that though...

sebmarkbage · 2016-10-31T17:18:29Z

@rricard If we dedupe, it should be deuping based on the class (the type field), not the fiber instance. It is not necessary to receive it multiple times for the same class.

However, I don't think the stack reconciler does this. So we can leave that problem for later. NBD. I'd just remove the deduplication so we can land the first version of this.

rricard · 2016-10-31T18:25:03Z

@sebmarkbage removed dedupe!

gaearon · 2016-10-31T18:37:44Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -96,6 +99,90 @@ module.exports = function(scheduleUpdate : (fiber: Fiber, priorityLevel : Priori
    return true;
  }

+  function checkClassInstance(workInProgress: Fiber, inst: any) {
+    const type = workInProgress.type;


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.

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

rricard · 2016-10-31T20:31:20Z

@gaearon Refactored in a getName helper function

gaearon · 2016-10-31T21:48:00Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

+
+    if (inst.state && (typeof inst.state !== 'object' || isArray(inst.state))) {
+      // TODO: Change this to be a warning.
+      throw new Error(`${getName(workInProgress, inst)}.state: must be set to an object or null`);


Can you please use invariant module for this? Otherwise the error text won't be automatically stripped out in production builds.

Ok, so that's why you have both! Invariant throws, warning goes in the console... Will do!

gaearon · 2016-10-31T21:48:29Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

+      );
+    }
+
+    if (inst.state && (typeof inst.state !== 'object' || isArray(inst.state))) {


Nitpicking but let's read inst.state once?

rricard · 2016-11-01T13:20:18Z

Well, I can't explain why the last build failed...

gaearon · 2016-11-01T13:55:49Z

Maybe it ran out of memory generating coverage or something like this.

rricard · 2016-11-01T13:58:59Z

@gaearon happened on an another build: https://travis-ci.org/facebook/react/builds/172282875

gaearon · 2016-11-01T17:38:41Z

I just found the errors are caused by a new script for tracking fiber errors.
Please rebase on master and then run

./scripts/fiber/record-tests

This will update the file tracking passing tests so we know which exactly are passing.

rricard · 2016-11-02T20:34:02Z

@gaearon done!

sebmarkbage · 2016-11-02T21:06:23Z

scripts/fiber/tests-failing.txt

@@ -87,6 +72,9 @@ src/renderers/art/__tests__/ReactART-test.js
 * resolves refs before componentDidMount
 * resolves refs before componentDidUpdate

+src/renderers/dom/__tests__/ReactDOMProduction-test.js
+* should throw with an error code in production


Hm. This is strange that this was passing before. This is probably a behavior change that we want to allow.

sebmarkbage · 2016-11-02T21:07:04Z

Can you rebase and we'll see if it passes the Travis CI tests?

Rejects Arrays, Strings & Numbers. Allows nulls.

- check the absence of getInitialState & getDefaultProps as methods - check the absence of propTypes & contextTypes at instance-level

Avoid rechecking while resuming

This is achieved by tracking the warning state in the Fiber structure.

rricard · 2016-11-05T10:18:36Z

@sebmarkbage you were right, this should be fixed now!

sebmarkbage · 2016-11-05T18:16:49Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

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

+  function getName(workInProgress: Fiber, inst: any): string {


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

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.

* Print a warning in fiber for lacking render method * Reject initial non-object state in Fiber Rejects Arrays, Strings & Numbers. Allows nulls. * Centralize fiber checks on ES6 Classes * Add classic & instance property checks on fiber - check the absence of getInitialState & getDefaultProps as methods - check the absence of propTypes & contextTypes at instance-level * Convert to normalized React `warning` calls * Support lifecycle typo detection in fiber * Get the complete warnings from the existing code * Only check classInstance once Avoid rechecking while resuming * Can warn component while resuming but only once This is achieved by tracking the warning state in the Fiber structure. * Remove warning deduplication * Factor name retrieval in a getName helper * Use invariant instead of throw * Read inst.state only once for the invariant * Fix condition on the instance state * Register failing/passing fiber tests

facebook-github-bot added the GH Review: review-needed label Oct 30, 2016

facebook-github-bot added CLA Signed labels Oct 30, 2016

rricard changed the title ~~[Work In Progress][Fiber] Complete ES6 Class Support~~ [Fiber] Complete ES6 Class related errors support Oct 30, 2016

facebook-github-bot added the CLA Signed label Oct 30, 2016

gaearon reviewed Oct 30, 2016

View reviewed changes

sebmarkbage reviewed Oct 30, 2016

View reviewed changes

sebmarkbage requested changes Oct 30, 2016

View reviewed changes

sebmarkbage added GH Review: needs-revision and removed GH Review: review-needed labels Oct 30, 2016

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Oct 30, 2016

gaearon reviewed Oct 31, 2016

View reviewed changes

gaearon self-assigned this Nov 1, 2016

sebmarkbage reviewed Nov 2, 2016

View reviewed changes

rricard added 2 commits November 5, 2016 11:13

Print a warning in fiber for lacking render method

306518c

Reject initial non-object state in Fiber

071b884

Rejects Arrays, Strings & Numbers. Allows nulls.

rricard added 13 commits November 5, 2016 11:13

Centralize fiber checks on ES6 Classes

9618961

Add classic & instance property checks on fiber

184a82a

- check the absence of getInitialState & getDefaultProps as methods - check the absence of propTypes & contextTypes at instance-level

Convert to normalized React warning calls

4a87efd

Support lifecycle typo detection in fiber

edb58c2

Get the complete warnings from the existing code

3c3f44b

Only check classInstance once

a69001c

Avoid rechecking while resuming

Can warn component while resuming but only once

102ece5

This is achieved by tracking the warning state in the Fiber structure.

Remove warning deduplication

2b0fc93

Factor name retrieval in a getName helper

89377af

Use invariant instead of throw

6a92c3f

Read inst.state only once for the invariant

f8289a7

Fix condition on the instance state

9ae84f1

Register failing/passing fiber tests

c8dc3c9

sebmarkbage reviewed Nov 5, 2016

View reviewed changes

sebmarkbage approved these changes Nov 5, 2016

View reviewed changes

sebmarkbage added GH Review: accepted and removed GH Review: review-needed labels Nov 5, 2016

sebmarkbage merged commit ba6b53a into facebook:master Nov 5, 2016

This was referenced Nov 8, 2016

Add notes for November, 3 reactjs/core-notes#36

Merged

[Fiber] Umbrella for remaining features / bugs #7925

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fiber] Complete ES6 Class related errors support #8156

[Fiber] Complete ES6 Class related errors support #8156

rricard commented Oct 30, 2016 •

edited

Loading

rricard commented Oct 30, 2016 •

edited

Loading

gaearon Oct 30, 2016

rricard Oct 30, 2016

rricard Oct 30, 2016

sebmarkbage Oct 30, 2016

sebmarkbage Oct 30, 2016

sebmarkbage Oct 30, 2016

sebmarkbage Oct 30, 2016

sebmarkbage Oct 30, 2016

rricard commented Oct 30, 2016

rricard commented Oct 30, 2016 •

edited

Loading

sebmarkbage commented Oct 31, 2016

rricard commented Oct 31, 2016 •

edited

Loading

gaearon Oct 31, 2016

rricard Oct 31, 2016

rricard commented Oct 31, 2016

gaearon Oct 31, 2016

rricard Oct 31, 2016

gaearon Oct 31, 2016

rricard commented Nov 1, 2016

gaearon commented Nov 1, 2016

rricard commented Nov 1, 2016

gaearon commented Nov 1, 2016

rricard commented Nov 2, 2016

sebmarkbage Nov 2, 2016

sebmarkbage commented Nov 2, 2016

rricard commented Nov 5, 2016

sebmarkbage Nov 5, 2016

sebmarkbage Nov 5, 2016

[Fiber] Complete ES6 Class related errors support #8156

[Fiber] Complete ES6 Class related errors support #8156

Conversation

rricard commented Oct 30, 2016 • edited Loading

rricard commented Oct 30, 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

Choose a reason for hiding this comment

rricard commented Oct 30, 2016

rricard commented Oct 30, 2016 • edited Loading

sebmarkbage commented Oct 31, 2016

rricard commented Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rricard commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rricard commented Nov 1, 2016

gaearon commented Nov 1, 2016

rricard commented Nov 1, 2016

gaearon commented Nov 1, 2016

rricard commented Nov 2, 2016

Choose a reason for hiding this comment

sebmarkbage commented Nov 2, 2016

rricard commented Nov 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rricard commented Oct 30, 2016 •

edited

Loading

rricard commented Oct 30, 2016 •

edited

Loading

rricard commented Oct 30, 2016 •

edited

Loading

rricard commented Oct 31, 2016 •

edited

Loading