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 bundledDependencies #1068

Merged
merged 1 commit into from
Nov 21, 2016
Merged

Remove bundledDependencies #1068

merged 1 commit into from
Nov 21, 2016

Conversation

fson
Copy link
Contributor

@fson 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

Copy link
Contributor

Choose a reason for hiding this comment

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

Good riddance

Copy link

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

If Travis passes lgtm.

@gaearon gaearon added this to the 0.8.0 milestone Nov 20, 2016
@fson fson merged commit e042634 into facebook:master Nov 21, 2016
@fson fson deleted the remove-bundled-deps branch November 21, 2016 11:26
jarlef pushed a commit to jarlef/create-react-app that referenced this pull request Nov 28, 2016
* Remove bundledDependencies
* Change the e2e scripts to use local file dependencies instead of
  bundledDependencies to test the packages
lexun added a commit to CodingZeal/create-react-app that referenced this pull request Dec 1, 2016
Disabled because npm/npm#12834

It will also be removed upstream soon:
facebook#1068
alexdriaguine pushed a commit to alexdriaguine/create-react-app that referenced this pull request Jan 23, 2017
* Remove bundledDependencies
* Change the e2e scripts to use local file dependencies instead of
  bundledDependencies to test the packages
@Timer Timer mentioned this pull request Feb 14, 2017
joaogarin referenced this pull request in jobiqo/recruiter_epiq_deps Mar 23, 2017
Bundled dependencies is having some weird structure on npm client implementation, so removing it to

work seamsless with npm and yarn
randycoulman pushed a commit to CodingZeal/create-react-app that referenced this pull request May 8, 2017
* Remove bundledDependencies
* Change the e2e scripts to use local file dependencies instead of
  bundledDependencies to test the packages
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants