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

Build: Upgrade to Yarn 3 #15682

Merged
merged 5 commits into from Jul 28, 2021
Merged

Build: Upgrade to Yarn 3 #15682

merged 5 commits into from Jul 28, 2021

Conversation

gaetanmaisse
Copy link
Member

@gaetanmaisse gaetanmaisse commented Jul 26, 2021

What I did

  • Upgraded to Yarn 3 (-> specific version, see Build: Upgrade to Yarn 3 #15682 (comment))
  • Add a missing dep in @storybook/core-common
  • Remove unneeded @types dependency
  • Simplify Yarn config in web-components-kitchen-sink

How to test

  • CI should be 🟢
  • Checkout this branch and try to run:
    - yarn bootstrap --core
    - cd examples/official-storybook && yarn && yarn storybook
    - cd ../web-components-kitchen-sink && yarn && yarn storybook

@nx-cloud
Copy link

nx-cloud bot commented Jul 26, 2021

Nx Cloud Report

CI ran the following commands for commit c316086. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@gaetanmaisse
Copy link
Member Author

The WC example, using a fancy portal: config doesn't agree with this upgrade 🙈

➤ YN0001: │ Error: Assertion failed: Writing attempt prevented to /tmp/storybook/addons/storyshots/storyshots-core/node_modules/@jest/transform which is outside project root: /tmp/storybook/examples/web-components-kitchen-sink
    at lB (/tmp/storybook/.yarn/releases/yarn-3.0.0.cjs:558:1626)
    at Hie (/tmp/storybook/.yarn/releases/yarn-3.0.0.cjs:558:2199)
    at Rje (/tmp/storybook/.yarn/releases/yarn-3.0.0.cjs:558:6585)
    at Uie.finalizeInstall (/tmp/storybook/.yarn/releases/yarn-3.0.0.cjs:543:24938)
    at async Fe.linkEverything (/tmp/storybook/.yarn/releases/yarn-3.0.0.cjs:303:13759)
    at async /tmp/storybook/.yarn/releases/yarn-3.0.0.cjs:306:4326
    at async xe.startTimerPromise (/tmp/storybook/.yarn/releases/yarn-3.0.0.cjs:275:3730)
    at async Fe.install (/tmp/storybook/.yarn/releases/yarn-3.0.0.cjs:306:4102)
    at async /tmp/storybook/.yarn/releases/yarn-3.0.0.cjs:348:14731
    at async Function.start (/tmp/storybook/.yarn/releases/yarn-3.0.0.cjs:275:2287)
➤ YN0000: └ Completed in 1s 317ms
➤ YN0000: Failed with errors in 7s 603ms

Comment on lines +50 to +51
"babel-plugin-macros": "3.1.0",
"fork-ts-checker-webpack-plugin": "6.2.13",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to set fixed versions because there are some incompatible versions range due to our webpack 4 & 5 mix in the monorepo (should be cleaned in SB 7)

@larixer
Copy link

larixer commented Jul 27, 2021

It is a bug in the node-modules linker, I didn't foresee that situations like this might happen with portals. I am going to submit a fix, however it will require a bit of work. The problem is that there are two different versions of @jest/transform in the dependency graph: 26.6.2 used by storyshots-core and 27.0.6 used by jest-core and babel-jest, etc. You can see this by running yarn why @jest/transform. Unfortunately @jest/transform@27.0.6 gets hoisted first to web-components-kitchen-sink/node_modules/@jest/transform and it blocks the way for @jest/transform@26.6.2 , thats why nm linker tries to place @jest/transform@26.6.2 inside storybook/addons/storyshots/storyshots-core/node_modules/@jest/transform, but this attempt is rightfully blocked by assertion - we don't want to write inside portals for security reasons.
The fix would be to give a priority to direct portal dependencies above all other dependencies, so that @jest/transform@26.6.2 be placed inside web-components-kitchen-sink/node_modules/@jest/transform first.

Until the nm linker fix lands, the workaround might be to reduce the number of @jest/transform versions used, if possible. I will update you when the fix will be ready.

@larixer
Copy link

larixer commented Jul 27, 2021

The fix is available here:
yarnpkg/berry#3183
Angular E2E tests are failing, but its not our fault (they use incorrect semver at the moment for published packages, they fixed this, but not published yet):
angular/angular-cli#21413

I have checked the fix on this branch and install finished without errors. To try the fix you can run:
yarn set version from sources --branch 3183
I'm going to add an integration test to this PR to prevent regressions

@gaetanmaisse
Copy link
Member Author

Awesome @larixer 👏🏻 ! I will update this PR with the provided "version" later today

@gaetanmaisse gaetanmaisse added the maintenance User-facing maintenance tasks label Jul 27, 2021
@gaetanmaisse gaetanmaisse marked this pull request as ready for review July 27, 2021 12:48
@gaetanmaisse
Copy link
Member Author

E2E statuses:

@gaetanmaisse gaetanmaisse requested review from shilman and a team July 27, 2021 12:53
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff @gaetanmaisse 🚀

@gaetanmaisse gaetanmaisse merged commit 19f32e3 into next Jul 28, 2021
@gaetanmaisse gaetanmaisse deleted the upgrade-to-yarn-3 branch July 28, 2021 07:10
@shilman shilman added this to the 6.4 PRs milestone Jul 28, 2021
@larixer
Copy link

larixer commented Aug 3, 2021

@gaetanmaisse FYI, I have tried to enable usage of hardlinks by Yarn 3 inside node_modules via nmMode: hardlinks-local setting, and the total Stroybook size on disk went from 3.3G to 2.3G for me while yarn bootstrap --core && cd examples/official-storybook && yarn && yarn storybook succeeded. Usage of hardlinks inside node_modules does not have performance cost and theoretically should not break anything, but your mileage may vary, of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants