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

Fixes couple of release blockers #5730

Merged
merged 3 commits into from
Sep 13, 2023
Merged

Fixes couple of release blockers #5730

merged 3 commits into from
Sep 13, 2023

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Sep 12, 2023

What's the problem this PR addresses?

  • CRA broke when we started required that --cwd be put before any other argument. CRA is mostly deprecated / unmaintained by now, but it has a decent amount of downloads (more than create-next-app), so it doesn't cost us much to special-case a fix for the v4 that we can then drop in v5.

  • When a machine had a hot cache for package X in both cache versions A and B (each variant having its own checksum), when migrating a project cache version from A to B, Yarn would mistakenly try to validate the variant B using the checksum from variant A.

    • We already have a mechanism to tolerate checksum changes when going from one cache key to the next, but before Cache tolerance for old archives #5564 we used to retrieve the cache key from the file name, whereas we now retrieve it from the lockfile instead. A branch of code that relied on the assumption that the cache key could be checked later became invalid, hence the problem.

How did you fix it?

  • The --cwd flag is now allowed (as a special case) at the end of yarn add.

  • I refactored the "is this locator compatible with the current cache key" function outside of getLocatorPath.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@arcanis arcanis merged commit ad8c95d into master Sep 13, 2023
@arcanis arcanis deleted the mael/fixes branch September 13, 2023 13:38
@arcanis arcanis mentioned this pull request Sep 15, 2023
3 tasks
arcanis added a commit that referenced this pull request Sep 15, 2023
**What's the problem this PR addresses?**

The cache fix merged in #5730 had a
bug that caused the global cache to be skipped when a package had no
checksum - which happens when there are no lockfile in the project. This
was detected by the perf regression visible on the dashboard:

<img width="1381" alt="image"
src="https://github.com/yarnpkg/berry/assets/1037931/11aed9d5-7946-418d-806a-6df78aee4734">

**How did you fix it?**

I fixed the condition (it was "use the cache if there's a checksum and
it's compatible", it should have been "use the cache if there's no
checksum, or it's a compatible one") and added tests to avoid further
regressions.

**Checklist**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [x] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant