-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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 bundledDependencies #1068
Conversation
fson
commented
Nov 20, 2016
- Remove bundledDependencies
- Change the e2e scripts to use local file dependencies instead of bundledDependencies to test the packages
* Remove bundledDependencies * Change the e2e scripts to use local file dependencies instead of bundledDependencies to test the packages
|
||
# This modifies package.json to copy all dependencies to bundledDependencies | ||
node ./node_modules/.bin/bundle-deps | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good riddance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why bundle-deps was removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of many issues with them due to npm's buggy client implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curiously, the bundled dependencies weren't really speeding up the installation either. I did a quick benchmark comparing the latest nightly build (without bundled deps) and 0.7.0 and there was no discernible difference in the time to create a new app. With a warmed up cache, the new version without bundled deps was actually outperforming the old version that had them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it was only true for earlier versions with a different dependency structure or something like this.
If Travis passes lgtm. |
* Remove bundledDependencies * Change the e2e scripts to use local file dependencies instead of bundledDependencies to test the packages
Disabled because npm/npm#12834 It will also be removed upstream soon: facebook#1068
* Remove bundledDependencies * Change the e2e scripts to use local file dependencies instead of bundledDependencies to test the packages
Bundled dependencies is having some weird structure on npm client implementation, so removing it to work seamsless with npm and yarn
* Remove bundledDependencies * Change the e2e scripts to use local file dependencies instead of bundledDependencies to test the packages