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

providesModule -> explicit requires for cross-package dependencies #9078

Merged
merged 4 commits into from
Mar 1, 2017

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Feb 28, 2017

This is one step closer to a CommonJS/Lerna set up.

I added some forwarding modules for Flow and Jest's sake. They have the same purpose as the symlinks Lerna or Yarn Workspaces would provide.

I ran a codemod to use explicit package paths for cross-package dependencies and unit tests.

The rest of the requires are still providesModule.

Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

LGTM :)

@gaearon
Copy link
Collaborator

gaearon commented Feb 28, 2017

Need to verify UMD builds still work correctly.
I'll take a look soon.

@trueadm
Copy link
Contributor

trueadm commented Feb 28, 2017

@gaearon maybe wait till you come into the office, I'd like you to step me through that process to see if there's anything I missed? Thanks man.

@gaearon
Copy link
Collaborator

gaearon commented Feb 28, 2017

Yea that was my plan!

@trueadm
Copy link
Contributor

trueadm commented Feb 28, 2017

@gaearon @sebmarkbage I've just tested the UMD bundles locally on your branch and they all look good to me!

Files that require modules from a different package than their own now
does so by the npm path name instead of the providesModule.
This is a bit lame but because of our module rewrite we need to white
list all the paths that we don't *don't* want to rewrite.
Copy link
Contributor

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

It feels weird to require react/lib/* from files that get packaged into react, but I suppose this makes sense.

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.

5 participants