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

Require instantiable components to extend React.Component #4663

Merged
merged 1 commit into from Sep 8, 2015

Conversation

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Aug 19, 2015

Fixes #4599.

@sophiebits
Copy link
Collaborator Author

@sophiebits sophiebits commented Aug 19, 2015

This almost works, except MockedComponent here doesn't end up with the flag:

MockedComponent = mocks.generateFromMetadata(metaData);

@sebmarkbage ideas?

@sophiebits sophiebits added this to the 0.14 milestone Aug 31, 2015
@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Sep 1, 2015

If you use an object, jest will mock it. You would potentially set it to an object during DEV or testing maybe?

@sophiebits sophiebits force-pushed the sophiebits:isreactclass branch from 5261d02 to 580cf7c Sep 1, 2015
@sophiebits
Copy link
Collaborator Author

@sophiebits sophiebits commented Sep 1, 2015

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Sep 1, 2015

Another thing we could do is:

element.type.prototype instanceof React.Component

That locks it into a by-reference equality (and therefore a single react package). Not sure if that is good or bad. Certainly more idiomatic.

@syranide
Copy link
Contributor

@syranide syranide commented Sep 1, 2015

That locks it into a by-reference equality (and therefore a single react package). Not sure if that is good or bad. Certainly more idiomatic.

Multiple React packages means it's likely they'll be different versions, so that's probably preferable? Mixing React packages seems like a bad idea (or a mistake) and being slapped with an error seems beneficial I would think.

@jimfb
Copy link
Contributor

@jimfb jimfb commented Sep 1, 2015

I like instanceof; feels cleaner than checking for magic properties.

@sophiebits
Copy link
Collaborator Author

@sophiebits sophiebits commented Sep 1, 2015

I thought the whole point (one of the whole points) of 0.14 (0.15 maybe) was that you can use more than one React.

@syranide
Copy link
Contributor

@syranide syranide commented Sep 1, 2015

@spicyj But not mixing between them I would assume?

@jimfb
Copy link
Contributor

@jimfb jimfb commented Sep 1, 2015

@spicyj You can use more than one react-renderer without using more than one react-core. React.Component lives in the shared core, and I think it's very reasonable to assume a single shared react-core.

sophiebits added a commit that referenced this pull request Sep 8, 2015
Require instantiable components to extend React.Component
@sophiebits sophiebits merged commit b01af40 into facebook:master Sep 8, 2015
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants