Warn if class has a render() method but doesn't extend React.Component #11168

swyxio · 2017-10-10T02:00:41Z

Hi! adding the fix for #10103. test/prettier/lint/flow all done and passing. (replaces PR #11149)

gaearon · 2017-10-10T11:32:41Z

Prettier is failing. Have you run yarn before running prettier?

swyxio · 2017-10-10T14:18:32Z

gosh really embarrassing. Looks like i didnt commit the prettified file. I have resubmitted and it should pass. Jeez I am going to be so embarrassed when I look back on this in a few months

gaearon

See changes below + we'll need a test for this.

gaearon · 2017-10-10T15:15:24Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+      warning(
+        !(workInProgress.type.prototype &&
+          workInProgress.type.prototype.render),
+        'Warning: Using a class with a render method as a function, ' +


Please remove Warning: , it gets added automatically.

gaearon · 2017-10-10T15:15:46Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

@@ -481,6 +481,12 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
    var value;

    if (__DEV__) {
+      warning(
+        !(workInProgress.type.prototype &&


This is a bit hard to follow. Let's just wrap it in an if block with the real condition, and then pass false to the warning call.

gaearon · 2017-10-10T15:17:57Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+        !(workInProgress.type.prototype &&
+          workInProgress.type.prototype.render),
+        'Warning: Using a class with a render method as a function, ' +
+          'did you forget to extend React.Component?',


I would tweak the wording to:

The <%s /> component appears to have a render method, but doesn't extend React.Component. This is likely to cause errors. Change %s to extend React.Component instead.

You’ll need to pass the component name (use getComponentName).

swyxio · 2017-10-10T20:56:06Z

thanks @gaearon . its not clear to me -where- to add the tests. is there a deeper test guide somewhere other than the 4 page Contributing docs?

closest i can find is ReactIncrementalSideEffects-test.js where you do test warnings but this issue we are testing has no side effects
I could also put it in ReactIncremental-test.js but it is not obvious to me where the natural home is because of lack of similar tests. Not sure if you guys care about the order of tests!!
ReactIncrementalErrorHandling-test.js seems to be for true error handling.

swyxio · 2017-10-10T22:32:23Z

added the test in ReactIncremental-test.js - no idea if this is the right place since i couldnt find similar tests to colocate this with but i figure i should just post it up for your comment in case it is acceptable!

gaearon · 2017-10-11T10:55:35Z

Sorry the structure is a bit confusing. We haven’t updated the file structure to be sensible yet after deleting the old engine.

But I’d put it here.

gaearon · 2017-10-11T10:56:14Z

Also the test is failing:

Expected spy to have been called with:

"Warning: The <ClassWithRenderNotExtended /> component appears to have a render method, but doesn't extend React.Component. This is likely to cause errors. Change ClassWithRenderNotExtended to extend React.Component instead." as argument 1,

but it was called with

"Warning: The <ClassWithRenderNotExtended /> component appears to have a render method, but doesn't extend React.Component.This is likely to cause errors. Change ClassWithRenderNotExtended to extend React.Component instead.".

Have you tried running it with npm test -- <filename>?

swyxio · 2017-10-11T13:40:48Z

seems i errantly deleted a space when I made the warning multiline. will fix and move as suggested.

Yeah is there a separate issue for restructuring the test file structure? "ReactIncremental" doesnt really seem to make sense anymore

gaearon · 2017-10-28T17:45:08Z

Sorry for the trouble again. We rearranged how files are laid out in the repo. Could you please reapply your changes on top of master?

merging in fb's changes

swyxio · 2017-10-29T01:36:26Z

I confess I'm not sure what's going on. I thought I could simply git pull your changes from master and git merge it into my branch, however when the checks run (as they have here) there are -hundreds- of test failures presumably because the files have been moved around. I'm not sure why the tests are not updated in my branch. I'm going to try junking my branch and simply re-creating the branch fresh from your master

swyxio · 2017-10-29T02:48:18Z

ok this one looks like it works. Woohoo!

gaearon · 2017-10-31T11:51:46Z

packages/react-dom/src/__tests__/ReactCompositeComponent-test.js

+      try {
+        ReactDOM.render(<ClassWithRenderNotExtended />, container);
+      } catch (e) {
+        expect(e).toEqual(new TypeError('Cannot call a class as a function'));


This only runs assertions if we get into catch. But what if we have a bug that causes us not to throw an error? Then the test doesn't assert anything.

gaearon · 2017-10-31T12:02:14Z

I made a few minor tweaks:

Made sure the test fails if we don't throw.
Simplified the condition that we check.

I think this is good to go. Thanks!

facebook#11168) Warn if class has a render() method but doesn't extend React.Component

facebook-github-bot added the CLA Signed label Oct 10, 2017

gaearon requested changes Oct 10, 2017

View reviewed changes

Merge pull request #1 from facebook/master

59d9c06

merging in fb's changes

added forgotExtendClass warning on new master

854daa9

gaearon reviewed Oct 31, 2017

View reviewed changes

gaearon added 2 commits October 31, 2017 11:56

Tweak the test to assert in all cases

e8d3851

Simplify the condition

97e3a77

gaearon approved these changes Oct 31, 2017

View reviewed changes

gaearon changed the title ~~add classWithRenderNotExtended warning~~ Warn if class has a render() method but doesn't extend React.Component Oct 31, 2017

gaearon merged commit 7f10fae into facebook:master Oct 31, 2017

gaearon mentioned this pull request Oct 31, 2017

Improve error when you've forgotten to extend React.Component #10103

Closed

swyxio deleted the forgotExtendWarning branch October 31, 2017 14:09

dependencies bot mentioned this pull request Nov 13, 2017

Update react-dom in / from 16.0.0 to 16.1.1 dropseed/jump#18

Merged

Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017

Warn if class has a render() method but doesn't extend React.Component (

ed86315

facebook#11168) Warn if class has a render() method but doesn't extend React.Component

gaearon mentioned this pull request Jan 8, 2018

Warn in server renderer if class doesn't extend React.Component but has a render() method #11991

Closed

renovate bot mentioned this pull request Feb 22, 2018

Update react monorepo packages to v16.2.0 mozilla/ensemble#28

Closed

This was referenced Mar 1, 2018

Update react-dom in / from 16.0.0 to 16.2.0 pcardune/redux-firebase-mirror#27

Open

Update react-test-renderer in / from 16.0.0 to 16.2.0 pcardune/redux-firebase-mirror#28

Open

This was referenced Mar 21, 2018

Update react to 16.3.2-abandoned sbardian/DebtTrackerAPI#15

Closed

Update react-dom to ^16.0.0-abandoned sbardian/DebtTrackerAPI#18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn if class has a render() method but doesn't extend React.Component #11168

Warn if class has a render() method but doesn't extend React.Component #11168

swyxio commented Oct 10, 2017

gaearon commented Oct 10, 2017

swyxio commented Oct 10, 2017

gaearon left a comment

gaearon Oct 10, 2017

gaearon Oct 10, 2017

gaearon Oct 10, 2017

swyxio commented Oct 10, 2017 •

edited

swyxio commented Oct 10, 2017 •

edited

gaearon commented Oct 11, 2017

gaearon commented Oct 11, 2017 •

edited

swyxio commented Oct 11, 2017 •

edited

gaearon commented Oct 28, 2017

swyxio commented Oct 29, 2017

swyxio commented Oct 29, 2017

gaearon Oct 31, 2017

gaearon commented Oct 31, 2017

Warn if class has a render() method but doesn't extend React.Component #11168

Warn if class has a render() method but doesn't extend React.Component #11168

Conversation

swyxio commented Oct 10, 2017

gaearon commented Oct 10, 2017

swyxio commented Oct 10, 2017

gaearon left a comment

Choose a reason for hiding this comment

gaearon Oct 10, 2017

Choose a reason for hiding this comment

gaearon Oct 10, 2017

Choose a reason for hiding this comment

gaearon Oct 10, 2017

Choose a reason for hiding this comment

swyxio commented Oct 10, 2017 • edited

swyxio commented Oct 10, 2017 • edited

gaearon commented Oct 11, 2017

gaearon commented Oct 11, 2017 • edited

swyxio commented Oct 11, 2017 • edited

gaearon commented Oct 28, 2017

swyxio commented Oct 29, 2017

swyxio commented Oct 29, 2017

gaearon Oct 31, 2017

Choose a reason for hiding this comment

gaearon commented Oct 31, 2017

swyxio commented Oct 10, 2017 •

edited

swyxio commented Oct 10, 2017 •

edited

gaearon commented Oct 11, 2017 •

edited

swyxio commented Oct 11, 2017 •

edited