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.

Sign up for GitHub

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

Jump to bottom

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

@fson
Remove bundledDependencies
e5ab354 
* Remove bundledDependencies
* Change the e2e scripts to use local file dependencies instead of
  bundledDependencies to test the packages
gaearon
gaearon reviewed Nov 20, 2016
View reviewed changes
tasks/release.sh

# 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
@thien-do thien-do mentioned this pull request Nov 21, 2016
Closed
@fson fson merged commit e042634 into facebook:master Nov 21, 2016
@fson fson deleted the remove-bundled-deps branch November 21, 2016 11:26
@fson fson mentioned this pull request Nov 22, 2016
Closed
jarlef pushed a commit to jarlef/create-react-app that referenced this pull request Nov 28, 2016
@fson @jarlef
Remove bundledDependencies (facebook#1068)
27d6e93 
* 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
@lexun
Remove bundledDependencies
a64ba0c 
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
@fson @alexdriaguine
Remove bundledDependencies (facebook#1068)
b2a9cad 
* 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
Closed
joaogarin referenced this pull request in jobiqo/recruiter_epiq_deps Mar 23, 2017
@joaogarin
fix(dependencies): Remove bundled dependencies
e27dd29 
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
@fson @randycoulman
Remove bundledDependencies (facebook#1068)
1f0bb7f 
* 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.
Reviewers

@camwest camwest camwest left review comments

@gaearon gaearon gaearon left review comments

Assignees
No one assigned
Labels
CLA Signed tag: internal
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fson @gaearon @camwest @facebook-github-bot