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

Remove indirection in checking propTypes #9358

Merged
merged 2 commits into from
Apr 7, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 6, 2017

TLDR: I’m removing react/lib/checkPropTypes import in favor of React.checkPropTypes public API access from renderer, and direct checkPropTypes access from the core.

Shared checkReactTypeSpec currently depends on react/lib/checkPropTypes which is gone in flat bundles. So this breaks in React Native sync process. In DOM, we currently ship two copies (in both React and ReactDOM flat bundles) but this breaks until RN is also a flat bundle. I could shim it but this seems like an easy fix because there’s a public API for this now.

The fix inside React package is just to import the internal checkPropTypes module directly.
The fix inside renderers is to use the public API (React.checkPropTypes).

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 6, 2017

Hmm this seems like the wrong way to do it because of a circular dep. Easier to kill this module and do it differently I think.

@gaearon gaearon changed the title Replace direct react/lib/ access with top-level React call Remove indirection in checking propTypes Apr 6, 2017
@gaearon
Copy link
Collaborator Author

gaearon commented Apr 6, 2017

OK should be ready for review!

@acdlite
Copy link
Collaborator

acdlite commented Apr 7, 2017

We can remove checkPropTypes and import from prop-types/checkPropTypes instead.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 7, 2017

Means we technically start to depend on it?

@acdlite
Copy link
Collaborator

acdlite commented Apr 7, 2017

Yeah we can remove ReactPropTypes too. I started a PR to migrate all the tests accordingly but haven't finished yet, will wrap up post-15.5 (tomorrow).

@acdlite
Copy link
Collaborator

acdlite commented Apr 7, 2017

Although, because of flat bundles it would just be a dev dependency, right?

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 7, 2017

Currently cjs flat bundles still use require for external deps (so that they can be deduped).

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 7, 2017

I'll get this in for now so I can continue work on RN flat bundling, happy to change later.

@gaearon gaearon merged commit a8d37a7 into facebook:master Apr 7, 2017
@gaearon gaearon deleted the use-proptypes-api branch April 7, 2017 14:23
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.

None yet

3 participants