Skip to content

Upgrade to Yarn 3#2657

Closed
srmagura wants to merge 15 commits intoemotion-js:mainfrom
srmagura:yarn3-attempt2
Closed

Upgrade to Yarn 3#2657
srmagura wants to merge 15 commits intoemotion-js:mainfrom
srmagura:yarn3-attempt2

Conversation

@srmagura
Copy link
Copy Markdown
Contributor

@srmagura srmagura commented Feb 20, 2022

PLEASE SQUASH

This is ready for review, but not for merge. The documentation site build issue detailed below needs to be resolved before merge.

After merging or checking out these changes, I recommend deleting all node_modules folders in the repository. git clean -dix is a good way to do this (deletes all ignored files).

Other than just being a nice modernization change, this should make installs noticeably faster.

  • Removed opencollective postinstall from postinstall script since Yarn 3 doesn't actually display this output. (At least not when running locally — it would get displayed in CI. Not sure why.)
  • Removed redundant call to yarn lint:check from main.yml workflow.

Caching in CI workflow

With the caching left virtually unchanged from how it worked with Yarn 1 (caching both .yarn/cache and node_modules), workspace-related stuff went awry on Windows which you can see here: https://github.com/emotion-js/emotion/runs/5264870316.

My understanding is that it is an antipattern to cache node_modules, so I changed the caching to use the cache functionality built into actions/setup-node. I believe this only caches .yarn/cache.

This change will make the workflows a bit slower to run since each workflow must complete the linking phase of yarn install. Is this OK? One way to reduce the number of GitHub Actions minutes we use would be to reduce the number of jobs. For example, make the dtslint job a single job instead of a matrix.

Gatsby build

I cannot get the Gatsby site to build no matter what I try after this upgrade... the error is:

failed Building production JavaScript and CSS bundles - 44.290s

 ERROR #98124  WEBPACK

Generating JavaScript bundles failed

Can't resolve 'module' in 'C:\Projects\OSS\emotion\node_modules\resolve\lib'

If you're trying to use a package make sure that 'module' is installed. If you're trying to use a local file make sure that the path is correct.

File: ..\node_modules\resolve\lib\normalize-options.js

I think it would be easiest to just get the Next.js website PR merged into main first. That will probably cause this error to go away.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Feb 20, 2022

⚠️ No Changeset found

Latest commit: af6d597

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Feb 20, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit af6d597:

Sandbox Source
Emotion Configuration

@srmagura srmagura requested a review from Andarist February 20, 2022 15:37
@Andarist
Copy link
Copy Markdown
Member

I cannot get the Gatsby site to build no matter what I try after this upgrade... the error is:

Eh, ye - this is bizarre. It looks like webpack is trying to bundle babel-plugin-macros/node_modules/resolve/lib/normalize-options.js but I don't get why it worked before and why it has started to fail after the upgrade. 😢

@srmagura
Copy link
Copy Markdown
Contributor Author

When I looked at normalize-options.js, it looked like Yarn 3 was patching it to make it work with PnP (even though we aren't using PnP). So that explains why the error just started occurring, but not I still do not know how to fix it other than merging the Next.js website PR. 😢

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 30, 2022

Codecov Report

Merging #2657 (8efaf8a) into main (26e4e3e) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 8efaf8a differs from pull request most recent head af6d597. Consider uploading reports for the commit af6d597 to get more accurate results

Impacted Files Coverage Δ
packages/babel-plugin-jsx-pragmatic/src/index.js 92.00% <0.00%> (-3.84%) ⬇️
packages/cache/src/index.js 96.80% <0.00%> (-1.00%) ⬇️
packages/sheet/src/index.js 100.00% <0.00%> (ø)
packages/styled/src/base.js 100.00% <0.00%> (ø)
packages/utils/src/index.js 100.00% <0.00%> (ø)
packages/serialize/src/index.js 100.00% <0.00%> (ø)
packages/react/src/class-names.js 100.00% <0.00%> (ø)
packages/css/src/create-instance.js 100.00% <0.00%> (ø)
packages/primitives-core/src/css.js 100.00% <0.00%> (ø)
packages/jest/src/create-serializer.js 100.00% <0.00%> (ø)
... and 11 more

@srmagura
Copy link
Copy Markdown
Contributor Author

This one is also a mess, and the changes were quite minimal. Just going to close this and redo it once #2571 gets merged.

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.

2 participants