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

Constructor error message #11395

Merged
merged 9 commits into from
Nov 1, 2017
Merged

Constructor error message #11395

merged 9 commits into from
Nov 1, 2017

Conversation

dylanapplegate
Copy link
Contributor

Addresses #11381.

name,
);

if (type.prototype && type.prototype.render !== undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's put the whole thing into if (!renderPresent) so we don't run those checks unnecessarily. And make it warning(false.

warning(
false,
'%s(...): No `render` method found on the returned component ' +
'instance: is the `constructor` defined well?',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's be explicit: "did you accidentally return an object from the constructor"

@gaearon
Copy link
Collaborator

gaearon commented Oct 31, 2017

Do you mind writing a test inside ReactCompositeComponent-test against react-dom instead?
That's where such tests currently are, for better or worse.

@dylanapplegate
Copy link
Contributor Author

Sure thing! I'll have an updated PR over later today.

ReactTestUtils.renderIntoDocument(<RenderTextInvalidConstructor />);
}).toThrow();

const error = console.error.calls.mostRecent().args[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's also assert we only emit one warning. Like other tests do by asserting on count.

}).toThrow();

const error = console.error.calls.mostRecent().args[0];

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: let's remove this newline

ReactTestUtils.renderIntoDocument(<RenderTestUndefinedRender />);
}).toThrow();

const error = console.error.calls.mostRecent().args[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same feedback

@gaearon gaearon merged commit 787c2ad into facebook:master Nov 1, 2017
@gaearon
Copy link
Collaborator

gaearon commented Nov 1, 2017

Looks great. Thanks!

@dylanapplegate
Copy link
Contributor Author

@gaearon No problem! Do you have any recommendations for my next issue? Maybe something a little more difficult?

@dylanapplegate dylanapplegate deleted the constructor-error-message branch November 1, 2017 21:16
@gaearon
Copy link
Collaborator

gaearon commented Nov 1, 2017

Adding Flow coverage to files that don't currently have it (and fixing Flow errors) would be greatly appreciated.

@dylanapplegate
Copy link
Contributor Author

Yep, I'm game for adding flow coverage. Is there a list I can work from? Also, would you want a PR for each file?

@gaearon
Copy link
Collaborator

gaearon commented Nov 1, 2017

No particular list, but you can go through packages and do this in the order you see files without @flow. PR per a few related files (especially if they import each other) would probably be best. And it's better to start with untyped files that are imported from already typed files. So that it's expanding one "island" of coverage instead of creating multiple independent "islands" that might conflict with each other at seams later.

@dylanapplegate
Copy link
Contributor Author

Understood! I'll get a PR over with the first set by the weekend, maybe sooner.

@dylanapplegate
Copy link
Contributor Author

@gaearon,
I created PR 11465 which includes Flow types for EventPluginHub. Let me know if I'm heading in the right direction. Also, I opened a Flow issue, 5320. I ran into a type problem that I resolved by reordering the union type.

@gaearon
Copy link
Collaborator

gaearon commented Nov 11, 2017

There's a few older PRs that I haven't gotten to reviewing yet, so this will take a little more time. I'm also on vacation next week. I'll likely pick it up after that!

Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
* Constructor test and fix complete

* Linters and prettier run

* Remove unnecessary checks

* Update error message

* Updat unit test

* prettier

* Tweak the check to be more specific

* Move tests to ReactCompositeComponent-test

* add error call count and remove line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants