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

Caches the Yarn resolution for faster installs #5270

Merged
merged 5 commits into from
Oct 3, 2018

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Oct 3, 2018

When running create-react-app, a good chunk of the time is spent resolving the dependency tree to satisfying versions. Since react-scripts uses pinned dependencies anyway (suggesting that you always want to use precise versions), this looks a bit unnecessary.

This PR adds a new script, compile:lockfile, meant to be ran manually every now and then. It will update the yarn.lock.cached file located in the create-react-app package, which will be copied into the app directory before running yarn add react-scripts (only if Yarn is being used, of course). Three cases then:

  • If create-react-app is up-to-date with the latest react-scripts, the resolution will be mostly instantaneous.
  • If create-react-app is completely desynchronized with the react-scripts being used, the resolution will be like now (the cached resolution will simply be ignored by Yarn).
  • If create-react-app is old-but-not-that-much compared to react-scripts, Yarn will be able to reuse at least some of the cached resolution, which will still give a perf boost.

@arcanis
Copy link
Contributor Author

arcanis commented Oct 3, 2018

Small note: I recommend to use Yarn 1.12+ to generate this file. It will be easier for users to then transition to the 2.0 later on (because of the integrity field that would be missing otherwise), and won't prevent people running lower versions from using the lockfile anyway.

@gaearon
Copy link
Contributor

gaearon commented Oct 3, 2018

Can you describe what happens when new react-scripts is released? Is cached lockfile in CRA ignored?

@arcanis
Copy link
Contributor Author

arcanis commented Oct 3, 2018

It's the third case:

  • If create-react-app is old-but-not-that-much compared to react-scripts, Yarn will be able to reuse at least some of the cached resolution, which will still give a perf boost.

Yarn will use as much as possible from the cached lockfile (so if both react-scripts@1.0.0 and react-scripts@1.1.0 depend on lodash@1.0.0, they will both be covered by the same lockfile), and will resolve from the npm registry whatever it cannot find (if react-scripts@1.2.0 depends on lodash@1.1.0, then lodash@1.0.0 fron the cached lockfile will simply be ignored and removed at the end of the install).

@gaearon
Copy link
Contributor

gaearon commented Oct 3, 2018

How often should we run compile:lockfile? Should it be part of our release script?

@arcanis
Copy link
Contributor Author

arcanis commented Oct 3, 2018

Would be best, yes, to ensure it's always up-to-date with the latest react-scripts available. Otherwise it can be run manually when you change a dependency version in react-scripts/package.json.

@gaearon
Copy link
Contributor

gaearon commented Oct 3, 2018

Can you add it to our release instructions in Contributing.md?

@arcanis
Copy link
Contributor Author

arcanis commented Oct 3, 2018

There seems to be a problem in the md formatting, investigating.

@Timer
Copy link
Contributor

Timer commented Oct 3, 2018

There seems to be a problem in the md formatting, investigating.

I recently turned on md formatting, the changes are fine given your're on the correct version of Prettier.
This is probably the first edit to that file post-adding md.

@arcanis
Copy link
Contributor Author

arcanis commented Oct 3, 2018

Oh ok! Should be fine then 👍

@gaearon gaearon merged commit 201079d into facebook:master Oct 3, 2018
@Timer Timer added this to the 2.0.4 milestone Oct 3, 2018
@gaearon
Copy link
Contributor

gaearon commented Oct 3, 2018

Awesome. We've wanted to do this for a long time but I wasn't sure how. This makes sense.

@AviVahl
Copy link

AviVahl commented Oct 4, 2018

@gaearon @arcanis I think the cached lockfile should not be copied in case --custom-scripts is provided.

@arcanis
Copy link
Contributor Author

arcanis commented Oct 4, 2018

@AviVahl It shouldn't matter - Yarn will simply resolve the custom package passed as parameter against what's on the registry, then use it to bootstrap the app (and use the cache if they match the transitive dependencies of your custom package).

Worst case: there will be no identical dependency between the custom package and the cache, in which case the install will just be slower (it won't be able to take advantage of the cache and will need to resolve everything as before).

Unless you've noticed something awry in practice?

@AviVahl
Copy link

AviVahl commented Oct 4, 2018

@arcanis It works, yes, but the behavior has changed. If any semver request matches by accident, it will be locked to the cached version.

While react-scripts accepts the fact that fourth party updates are now also locked (to the time of the last compile:lockfile), some people out there creating custom scripts (like me) want upstream package security updates without having to wait for create-react-app to publish a new cached-lockfile.

I think the approach is flawed, because it also means you guys will have to release a new create-react-app version to update locked semver requests. And, for my understanding, that specific package should have less frequent releases, due to people installing it in global (even with npx being available).

@arcanis
Copy link
Contributor Author

arcanis commented Oct 4, 2018

I'm not sure I understand what you mean. Let's take scenarios:

  • There's no vulnerability, but you want to live on the edge. Then you can remove the lockfile once in your project folder and run yarn install again to upgrade everything. Note that because react-scripts pins its own dependencies directly within its package.json you'll only be able to upgrade the transitive dependencies (second level or more). I'm not sure that's super valuable? You also won't have any guarantee that anything will actually get updated, because of possible optimizations (cf the remark I made in the third scenario).

  • A react-scripts dependency is vulnerable. Since react-scripts already uses pinned dependency in its package.json, a new release of react-scripts is made to bump the version of the vulnerable dependency to a more recent one. Since the range isn't the same, old versions of create-react-app won't use the cache to resolve it and all new applications created using the newest react-scripts will correctly use the fixed release. All prior react-scripts have the vulnerable release pinned in their dependencies, so nothing changes: they would receive the vulnerable release even without resolution cache.

  • A dependency of a dependency of react-scripts is vulnerable. Two possibilities: either the first-level dependency has been updated to prevent the use of the vulnerable second-level dependency (in which case we can release a new react-scripts, cf first scenario), or is hasn't. In the second case, even without the lockfile cache, nothing guarantees that Yarn won't pick up the vulnerable version (even if there's a newer one): it could try to be smart and mutualize the dependency (for example if the vulnerable dependency version satisfies two ranges from the dependency tree but latest only matches one, it's probable that Yarn will prefer to use the "wrong" one).

That said, while I'm not sure I see the problem in having --custom-scripts use the cached versions (or, rather, I don't see what it really solves not to do it), I never used this option myself so I don't have a good idea of the way people use it, so I might miss an important thing 🙂

@AviVahl
Copy link

AviVahl commented Oct 4, 2018

My use case matches third-scenario, no forced upgrade from first-level (although it uses ^ for second-level, and that one also uses carets).

I'm creating avi-scripts. It is a custom/forked react-scripts. My instructions tell people to use create-react-app with --scripts-version avi-scripts.

In my avi-scripts, I specify dependencies with carets (^), so new users always get latest patches and minor versions. Majors are up to me to upgrade.

Without this PR, every time a new user comes to try out my scripts, he'll get both 1st-level and every-other-level resolved "fresh" (according to semver requests). Yarn might optimize it one way or another, but resolution still happens, so there's a chance a fixed upstream 10th-level dependency gets updated.

With this PR, my 1st-level deps are resolved "fresh", as I use carets while the original react-scripts uses exact versions. In most cases, this will trigger a re-resolution, as the yarn-cache.lock requests don't match mine. My 2nd-level deps (and on) are subject to be locked by the yarn-cache.lock in create-react-app. It happens when one of the semver requests matches one in the lockfile by accident.

Regarding "nothing guarantees that Yarn won't pick up the vulnerable version (even if there's a newer one)" - you are correct. But with this PR, it is guaranteed that it will pick up the exact same vulnerable version unless the semver request changes (in 2nd-level+ deps) or create-react-app releases a new version (with a new cached yarn.lock).

I understand the user can always go and delete his lock file and regenerate it from scratch, but the behavioral change is regarding the first install. Meaning usage of create-react-app to create a new project. I would even argue that most beginners don't touch their package.json/yarn.lock, so which versions are installed at that point matters.

Is my understanding of the works here correct? Are my assumptions?

@arcanis
Copy link
Contributor Author

arcanis commented Oct 4, 2018

I think your understanding is correct. One remark though:

But with this PR, it is guaranteed that it will pick up the exact same vulnerable version unless the semver request changes (in 2nd-level+ deps) or create-react-app releases a new version (with a new cached yarn.lock).

I think this is the crux of the problem here. But at the same time, I wonder if it's really one - the use case you mention is: "a vulnerable dependency won't be upgraded to a safe one". Couldn't that be reworded as "a safe dependency won't be upgraded to a vulnerable one"? Or, more likely: "a dependency we're sure is compatible won't be upgraded to something potentially breaking" (as sometimes happens when semver isn't entirely respected, or when a bug creeps in).

So I agree with your assessment, but I'm not entirely sure whether this behavior should be considered as being positive or negative 🤔

@AviVahl
Copy link

AviVahl commented Oct 4, 2018

Yes, it works both ways. That's probably why my first instinct was to change this just for custom scripts, where one may find a different, yet overlapping, dep tree.

If we had the exact same dependencies, I might have preferred this approach.

My experience has taught me that the same version can be good for one dep tree, and broken in another (where different features are used). I find forcing an initial lockfile from a completely different package problematic.

Am I going to find myself making a PR updating just the cached lockfile of create-react-app to fix an issue in my custom scripts package? Time will tell... :)

@gaearon
Copy link
Contributor

gaearon commented Oct 4, 2018

FWIW I don't mind limiting the lockfile thing only for the default package. Want to send a PR?

@AviVahl
Copy link

AviVahl commented Oct 4, 2018

@gaearon done.

@timsnadden
Copy link

What about the case where a user has a custom registry set for yarn? I suggest that in this case they should be excluded from getting the cached file. In my organisation users can't get to https://registry.yarnpkg.com/

@arcanis
Copy link
Contributor Author

arcanis commented Oct 31, 2018

Related: yarnpkg/yarn#5892

I guess the lockfile could be disabled through an option, like --no-resolution-cache or similar? Detecting the Yarn registry can also be automated but it might be a bit annoying (you need to parse yarn config list -s, which unfortunately isn't strict json).

@timsnadden
Copy link

What about execSync('yarn config get registry').toString() === 'https://registry.yarnpkg.com'; or similar?

@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
This pull request was closed.
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.

6 participants