-
Notifications
You must be signed in to change notification settings - Fork 233
chore: update yarn to v4 #5010
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
chore: update yarn to v4 #5010
Conversation
|
b92016a
to
901848e
Compare
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
Pull Request Test Coverage Report for Build 12776475618Details
💛 - Coveralls |
27a63f7
to
bafc5e9
Compare
a8c9e9c
to
1fe0168
Compare
2b3a589
to
7186094
Compare
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had two different versions being pinned—one byicons
(.17
) and another by icons-workflow
(.16
). In Yarn Classic, both were resolved to the lowest version, but in Yarn V4, they were resolved to the highest version. This difference caused some regressions. Pinning both to the same (older, lower) version fixed the issue.
downstream: | ||
steps: | ||
- checkout | ||
# - restore_cache: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had commented these lines out earlier to cache-bust. Although it doesn’t provide a significant benefit (just sub-minute run time improvements), I’ve re-added it.
cacheFolder: ~/.cache/yarn | ||
nodeLinker: node-modules | ||
yarnPath: .yarn/releases/yarn-4.6.0.cjs | ||
enableScripts: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enableScripts
is needed while we still have pre/post install commands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this comment with a @todo remove when pre/post install commands are moved to the new yarn 4 recommended pattern
feel free to wordsmith that lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call - just did! :)
- [Storybook](https://${branchSlug}--spectrum-web-components.netlify.app/storybook/)`; | ||
- [Storybook](https://${branchSlug}--spectrum-web-components.netlify.app/storybook/) | ||
// If there are changed packages, add a section with visual regression test results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should always have links for VRTs posted. For example, some dependency changes don’t trigger getChangedPackages()
, but they can still cause regressions. Since the VRTs run regardless, it’s just a matter of ensuring we see the links.
command: yarn docs:build | ||
- run: | ||
name: Build Storybook | ||
command: yarn storybook:build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have no clue how happy this makes me!! thank you for breaking the steps out and moving away from an overly complex command!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks perfect! A good, focused update to the latest package. I haven't pulled down locally but the code itself is approved pending validations.
e787b2b
to
2c6b3de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good work 🚀
Description
This PR upgrades our package management from Yarn Classic to Yarn 4.6.0 and modernizes our build pipeline.
Motivation and context
Our build pipeline needed some love (aka modernization) to take advantage of newer package management features and to align our project with our upstream Spectrum CSS dependency. The upgrade to Yarn 4 should provide better dependency resolution, improved performance, and more reliable builds. (Also, no more
wireit
warning logs filling up the terminal!)Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main
.