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

Jump to bottom

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

Merged
merged 4 commits into from Oct 31, 2017
Merged

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

merged 4 commits into from Oct 31, 2017

Conversation

swyxio
Copy link
Contributor

@swyxio swyxio commented Oct 10, 2017

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

@gaearon
Copy link
Collaborator

gaearon commented Oct 10, 2017

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

@swyxio
Copy link
Contributor Author

swyxio commented Oct 10, 2017

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
gaearon requested changes Oct 10, 2017
View reviewed changes
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

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

src/renderers/shared/fiber/ReactFiberBeginWork.js Outdated
warning(
!(workInProgress.type.prototype &&
workInProgress.type.prototype.render),
'Warning: Using a class with a render method as a function, ' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove Warning: , it gets added automatically.

src/renderers/shared/fiber/ReactFiberBeginWork.js Outdated
@@ -481,6 +481,12 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
var value;

if (__DEV__) {
warning(
!(workInProgress.type.prototype &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

src/renderers/shared/fiber/ReactFiberBeginWork.js Outdated
!(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?',
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

swyxio commented Oct 10, 2017 •
edited

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
Copy link
Contributor Author

swyxio commented Oct 10, 2017 •
edited

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

gaearon commented Oct 11, 2017

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

gaearon commented Oct 11, 2017 •
edited

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
Copy link
Contributor Author

swyxio commented Oct 11, 2017 •
edited

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

gaearon commented Oct 28, 2017

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?

@swyxio
Merge pull request #1 from facebook/master
59d9c06 
merging in fb's changes
@swyxio
Copy link
Contributor Author

swyxio commented Oct 29, 2017

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
Copy link
Contributor Author

swyxio commented Oct 29, 2017

ok this one looks like it works. Woohoo!

gaearon
gaearon reviewed Oct 31, 2017
View reviewed changes
packages/react-dom/src/__tests__/ReactCompositeComponent-test.js Outdated
try {
ReactDOM.render(<ClassWithRenderNotExtended />, container);
} catch (e) {
expect(e).toEqual(new TypeError('Cannot call a class as a function'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

gaearon commented Oct 31, 2017

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!

@gaearon gaearon changed the title add classWithRenderNotExtended warning Warn if class has a render() method but doesn't extend React.Component Oct 31, 2017
@gaearon gaearon merged commit 7f10fae into facebook:master Oct 31, 2017
@gaearon gaearon mentioned this pull request Oct 31, 2017
Closed
@swyxio swyxio deleted the forgotExtendWarning branch October 31, 2017 14:09
This was referenced Nov 9, 2017
Closed
Closed
Closed
Merged
Merged
Merged
Merged
@dependencies dependencies bot mentioned this pull request Nov 13, 2017
Merged
Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
@swyxio @Ethan-Arrowood
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 gaearon mentioned this pull request Jan 8, 2018
Closed
This was referenced Feb 2, 2018
Closed
Open
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Merged
Closed
@renovate renovate bot mentioned this pull request Feb 22, 2018
Closed
This was referenced Mar 1, 2018
Open
Open
This was referenced Mar 21, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaearon gaearon gaearon approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@swyxio @gaearon @facebook-github-bot