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

check if npm properly supports long cache-min and enable if so #375

Closed
wants to merge 1 commit into from

Conversation

apaleslimghost
Copy link
Contributor

Implements #373. @gaearon

@ghost
Copy link

ghost commented Aug 5, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@ghost
Copy link

ghost commented Aug 5, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ghost ghost added the CLA Signed label Aug 5, 2016
@apaleslimghost
Copy link
Contributor Author

Criteria from #373:

  1. New requirement: create-react-app my-app should not crash offline if you ran it at least once before. It should be able to use locally cached react-scripts in this case, if it exists.
  2. Bonus: create-react-app my-app should use cached react-scripts if it knows the latest npm version is cached locally. In other words, even if you are online, we shouldn’t just refetch the whole thing if we have it locally and we know it is the latest version. This is not required but would make a big difference for most people.
  3. Acceptable tradeoff: we are fine with not installing the latest transitive dependencies. In other words it’s fine if react-scripts depends on [email protected], then [email protected] comes out, but we keep using the cached [email protected].
  1. If create-react-app has been run once, react-scripts and its dependencies will be in npm's cache, and --cache-min=Infinity will install them
  2. --cache-min=Infinity only hits the network if a version is explicitly requested that is not in npm's cache.
  3. This will be the case unless react-scripts explicitly requests the new versions.

@apaleslimghost
Copy link
Contributor Author

I'm looking into the test failures.

@apaleslimghost
Copy link
Contributor Author

Hmm, the tests run fine locally with the same Node & npm versions as Travis. Possibly platform-specific? I'll try it on a Linux box tomorrow.

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

Maybe you can’t use --cache-min with a tarball?

`npm install --save-dev --save-exact --cache-min=Infinity /home/travis/build/facebookincubator/create-react-app/react-scripts-0.3.0-alpha.tgz` failed

@apaleslimghost
Copy link
Contributor Author

This is console.log(args) in run when I run the tests locally:

[ 'install',
  '--save-dev',
  '--save-exact',
  '--cache-min=Infinity',
  '/Users/matthew.brennan/Projects/create-react-app/react-scripts-0.3.0-alpha.tgz' ]

Either it's a crossplatform issue (which would be sad) or it's an npm race condition (which would be sadder).

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

I like to test linux stuff on nitrous.io

@ghost ghost added the CLA Signed label Aug 5, 2016
@apaleslimghost
Copy link
Contributor Author

Tests run fine on my Linux vps. Could still be something strange about the Travis env i guess? Note the actual npm error is:

npm ERR! Error extracting /home/travis/.npm/minimatch/2.0.1/package.tgz archive: ENOENT: no such file or directory, open '/home/travis/.npm/minimatch/2.0.1/package.tgz'

which does sound similar to some race conditions i've encountered in npm before

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

Mmm can we just try --cache-min and fallback to regular install if it fails?
Like described here: npm/npm#2568 (comment)

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

Also what about

Existing requirement: create-react-app my-app should use the most recent version of react-scripts if you are online. We are not changing this: we always want the users to get the most recent one.

?

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

@quarterto Would you be interested in finishing this up? I think it’ll make a lot of difference for many users.

@ghost ghost added the CLA Signed label Sep 2, 2016
@apaleslimghost
Copy link
Contributor Author

Hey, sorry, I'll take another look this weekend

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

Assigning this to @vjeux for review when you’re ready.
Please don’t hesitate to ping him.

@ghost ghost added the CLA Signed label Sep 2, 2016
@vjeux
Copy link
Contributor

vjeux commented Sep 2, 2016

👍

@apaleslimghost
Copy link
Contributor Author

Quick update: I didn't have much time at the weekend to work on this, but I can at least make the tests fail consistently with the same error as Travis now (which is a separate error to the original one so I'm not sure that's an improvement 😕 )

@gaearon gaearon mentioned this pull request Sep 24, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2016

Going to close since this is getting stale.
#373 is still open so if somebody wants to pick it up, feel free to!

@gaearon gaearon closed this Sep 30, 2016
@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.

3 participants