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

Unify the way we fork modules #11711

Merged
merged 5 commits into from
Nov 30, 2017
Merged

Unify the way we fork modules #11711

merged 5 commits into from
Nov 30, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 30, 2017

This unifies the ad hoc way we fork modules so it's easy to see which modules are forked under which conditions, and how to add new forks. This will help us get rid of the existing external dynamic injection that we still rely on in www (after this is merged).

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 30, 2017

Verified the first commit doesn't change a single byte in the bundles.

@bvaughn
Copy link
Contributor

bvaughn commented Nov 30, 2017

❤️ This looks great, Dan

@gaearon gaearon changed the title [WIP] Unify the way we fork modules Unify the way we fork modules Nov 30, 2017
@gaearon
Copy link
Collaborator Author

gaearon commented Nov 30, 2017

The second commit adds my custom plugin for replacing forks since rollup-plugin-alias doesn't quite work for us (not sure if bug or intentional). rollup/rollup-plugin-alias#34

I figured more control is nicer anyway since it's a pretty important part.

@gaearon gaearon requested a review from bvaughn November 30, 2017 02:07
This does exactly what we need and doesn't suffer from rollup/rollup-plugin-alias#34.
@gaearon
Copy link
Collaborator Author

gaearon commented Nov 30, 2017

Should be ready for review.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 30, 2017

Verified the final changes don't change the build either.

I settled on calling them "forks" since we already have a different concept of "shims".
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

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.

4 participants