chore: migrate from yarn to pnpm #4897

cardoso · 2024-11-20T20:59:44Z

Details

I'm still going through all scripts and haven't touched CI yet, but this is mostly working.

I don't know what external workflows might depend on things like the versions being spelled out in package.json and could break by using workspace:* and others. I initially made it work without that feature, so I can revert back to spelled out versions if needed.

Since pnpm doesn’t hoist dependencies to the root by default, it uncovers several issues similar to

fix(rollup-plugin): add @lwc/shared dep #4415

I’m submitting fixes for those separately to make sure they are correct.

I recommend cloning in a separate folder or running git clean -dfX before trying this out.

pnpm version used: 9.14.2.

Closes #4426

Top-level package.json scripts I tested so far:

Does this pull request introduce a breaking change?

😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

🤞 No, it does not introduce an observable change.

GUS work item

scripts/tasks/generate-license-files.mjs

packages/@lwc/integration-tests/README.md

nolanlawson

This is looking good so far. I'm surprised so many changed are needed for pnpm, but I do like the workspace:* thing.

One thing missing: this code to build the benchmarks will need to run yarn or pnpm:

lwc/packages/@lwc/perf-benchmarks/scripts/build.js

Lines 117 to 120 in 4df361b

    
           'yarn --immutable', 
        
           // bust the Tachometer cache in case these files change locally 
        
           `echo '${directoryHash}'`, 
        
           'yarn build:performance:components',

The reason is that it is going to compare the current master against an arbitrary commit from the past. Since that arbitrary commit might be using yarn, I'd suggest checking if [ -f yarn.lock ] inline.

cardoso · 2024-11-21T19:26:44Z

@nolanlawson thank you for looking into this! I'll see what I can do about the benchmark scripts. I left them for last since it takes a while to fully run.

The changes in src are mostly due to the non-flat structure of node_modules, which actually helps figure out some "phantom" and cyclic dependencies since they break pnpm i or type-checking. I'm trying to move the changes to separate PRs though.

nolanlawson

LGTM overall. It'll take some muscle memory for me to remember to type pnpm instead of yarn (also a bit more awkward to type 😆) but overall this is a great change. We were sticking with yarn v1 for way too long.

Also: the Tachometer tests work now! I guess we will just "shamefully hoist" 🤷

Thanks a lot!! ❤️

.npmrc

packages/@lwc/perf-benchmarks/scripts/build.js

nolanlawson · 2024-11-25T21:57:31Z

/nucleus test

Co-authored-by: Nolan Lawson <[email protected]>

nolanlawson · 2024-11-25T22:51:22Z

/nucleus test

nolanlawson · 2024-11-25T23:06:38Z

I can repro the above SauceLabs issue:

  require() of ES Module /Users/nlawson/workspace/lwc/node_modules/.pnpm/[email protected]/node_modules/got/dist/source/index.js from 
/Users/nlawson/workspace/lwc/node_modules/.pnpm/[email protected]/node_modules/saucelabs/build/index.js not 
supported.
Instead change the require of /Users/nlawson/workspace/lwc/node_modules/.pnpm/[email protected]/node_modules/got/dist/source/index.js in 
/Users/nlawson/workspace/lwc/node_modules/.pnpm/[email protected]/node_modules/saucelabs/build/index.js to a 
dynamic import() which is available in all CommonJS modules.

I'll dig into it since you need SauceLabs credentials in order to repro. Not sure why the pnpm change exposed this.

nolanlawson · 2024-11-25T23:42:22Z

After some fruitless debugging, I am quite lost. I cannot even figure out how to run the karma scripts with --inspect-brk so that I can debug. pnpm seems to be doing some magic under the hood that is causing weird issues like the ESM/CJS compat issue above. (BTW you can repro in the integration-karma dir using pnpm run start.)

Maybe we can switch to pnpm once require(esm) is shipped in Node v22. I am also still fine with upgrading to latest npm or latest yarn. Thanks again for all your help on this!

cardoso · 2024-11-25T23:56:07Z

@nolanlawson that's weird... I can't repro here. pnpm run start works fine in integration-karma.

cardoso · 2024-11-26T00:04:15Z

@nolanlawson actually, the pinned got version wasn't compatible with sauce. I unpinned it and that issue should be fixed.

This wasn't pinned originally, so I don't think it should block this PR.

packages/@lwc/integration-karma > [email protected] > [email protected] > [email protected] > [email protected] > [email protected]

Ref: GHSA-pfrx-2q88-qq97

This reverts commit 36bc724.

nolanlawson · 2024-11-26T00:05:24Z

Ahhh, that makes sense! I didn't consider that it was the overrides that did it.

BTW I opened #4946 to fix the subdir issue. No need to do it here.

I'm going to do some research into how to do inspect-brk with pnpm but other than that I guess we are good to go! Thanks. 🙇

nolanlawson · 2024-11-26T00:06:11Z

/nucleus test

cardoso · 2024-11-26T00:07:57Z

@nolanlawson maybe you need to do pnpm [command] -- --inspect-brk? pnpm doesn't forward the args the same way as yarn v1.

nolanlawson · 2024-11-26T00:16:54Z

Figured out the trick. To run the karma tests in the integration-karma dir, you can do:

NODE_OPTIONS=--inspect-brk ../../../node_modules/.bin/karma start \
    ./scripts/karma-configs/test/local.js --single-run --browsers ChromeHeadless

This is fine – it's just "one of those things you need to know" for pnpm I guess. The ../../../node_modules/.bin/karma is converted by pnpm from a Node script into a bash script (🤯) so you can also modify the node command inside of it to add --inspect-brk.

nolanlawson · 2024-11-26T00:17:26Z

/nucleus test

nolanlawson · 2024-11-26T00:22:23Z

/nucleus test

nolanlawson · 2024-11-26T00:28:11Z

Ugggghh... I just remembered something. Our internal tool for testing downstream repos, Nucleus, does not support pnpm. It only supports npm and yarn. 😞

I think we will either need to switch to npm or latest yarn. Sorry for the runaround @cardoso – I totally forgot until just now.

My personal preference is for latest npm. Don't feel obligated – I can do the conversion.

cardoso · 2024-11-26T00:33:08Z

@nolanlawson ouch! That was close 😅. Yeah, since I don't have access to nucleus, I'll leave it to you. Not sure if anything here can be reused, so feel free to close this PR.

nolanlawson · 2024-11-26T18:28:50Z

Closing for now, would be happy to revisit later!

cardoso added 4 commits November 20, 2024 15:33

chore: migrate public packages to pnpm

5134ddc

chore: replace yarn with pnpm in scripts

a4c624b

chore: migrate perf-benchmark-components to pnpm

bcf6544

fix: type and module resolution in tests

7856b08

cardoso requested a review from a team as a code owner November 20, 2024 20:59

salesforce-cla bot added the cla:signed label Nov 20, 2024

cardoso added 3 commits November 20, 2024 18:18

fix: karma tests

0b6ca4a

fix: integration-tests

ee30eab

fix: generate-license-files

3a31dc4

cardoso commented Nov 20, 2024

View reviewed changes

scripts/tasks/generate-license-files.mjs Outdated Show resolved Hide resolved

cardoso added 7 commits November 20, 2024 21:10

chore: use pnpm workspace protocol

e2a255d

chore: remove vite-tsconfig-paths

724449a

fix: type errors in test

577c896

fix: release:version

a938e90

fix: run pnpm audit --fix

25c3772

fix: type resolution in test files

826c112

fix: revert type change

afa6925

fix: migrate all usages and mentions of yarn to pnpm

0f17ab2

cardoso commented Nov 21, 2024

View reviewed changes

packages/@lwc/integration-tests/README.md Show resolved Hide resolved

cardoso added 2 commits November 21, 2024 07:21

fix: integration-tests

9e736f0

Merge branch 'master' of https://github.com/salesforce/lwc into pnpm

5ab58d6

nolanlawson reviewed Nov 21, 2024

View reviewed changes

cardoso added 3 commits November 21, 2024 16:02

Merge branch 'master' of https://github.com/salesforce/lwc into pnpm

a32cfcc

chore: revert license script to js

a45d383

fix: revert more unneeded changes

e25143b

nolanlawson approved these changes Nov 25, 2024

View reviewed changes

.npmrc Show resolved Hide resolved

packages/@lwc/perf-benchmarks/scripts/build.js Show resolved Hide resolved

nolanlawson mentioned this pull request Nov 25, 2024

[DO NOT MERGE] Trigger CI for #4897: chore: migrate from yarn to pnpm #4941

Closed

chore: add comment to .npmrc

7856a30

Co-authored-by: Nolan Lawson <[email protected]>

nolanlawson mentioned this pull request Nov 25, 2024

[DO NOT MERGE] Trigger CI for #4897: chore: migrate from yarn to pnpm #4944

Closed

chore: pin with volta in sub-dirs where we execute stuff

36bc724

fix: unpin got version

304be5e

Revert "chore: pin with volta in sub-dirs where we execute stuff"

9411269

This reverts commit 36bc724.

nolanlawson mentioned this pull request Nov 26, 2024

[DO NOT MERGE] Trigger CI for #4897: chore: migrate from yarn to pnpm #4947

Closed

nolanlawson mentioned this pull request Nov 26, 2024

[DO NOT MERGE] Trigger CI for #4897: chore: migrate from yarn to pnpm #4948

Closed

nolanlawson added 2 commits November 25, 2024 16:20

chore: fix github actions

9705a91

chore: do it DRYer

bee52c2

nolanlawson mentioned this pull request Nov 26, 2024

[DO NOT MERGE] Trigger CI for #4897: chore: migrate from yarn to pnpm #4949

Closed

This was referenced Nov 26, 2024

chore: switch to npm #4950

Closed

chore(perf-benchmarks): avoid yarn --immutable #4951

Merged

nolanlawson closed this Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: migrate from yarn to pnpm #4897

chore: migrate from yarn to pnpm #4897

cardoso commented Nov 20, 2024 •

edited

Loading

nolanlawson left a comment

cardoso commented Nov 21, 2024

nolanlawson left a comment

nolanlawson commented Nov 25, 2024

nolanlawson commented Nov 25, 2024

nolanlawson commented Nov 25, 2024

nolanlawson commented Nov 25, 2024

cardoso commented Nov 25, 2024

cardoso commented Nov 26, 2024

nolanlawson commented Nov 26, 2024 •

edited

Loading

nolanlawson commented Nov 26, 2024

cardoso commented Nov 26, 2024

nolanlawson commented Nov 26, 2024

nolanlawson commented Nov 26, 2024

nolanlawson commented Nov 26, 2024

nolanlawson commented Nov 26, 2024

cardoso commented Nov 26, 2024

nolanlawson commented Nov 26, 2024

	'yarn --immutable',
	// bust the Tachometer cache in case these files change locally
	`echo '${directoryHash}'`,
	'yarn build:performance:components',

chore: migrate from yarn to pnpm #4897

chore: migrate from yarn to pnpm #4897

Conversation

cardoso commented Nov 20, 2024 • edited Loading

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

nolanlawson left a comment

Choose a reason for hiding this comment

cardoso commented Nov 21, 2024

nolanlawson left a comment

Choose a reason for hiding this comment

nolanlawson commented Nov 25, 2024

nolanlawson commented Nov 25, 2024

nolanlawson commented Nov 25, 2024

nolanlawson commented Nov 25, 2024

cardoso commented Nov 25, 2024

cardoso commented Nov 26, 2024

nolanlawson commented Nov 26, 2024 • edited Loading

nolanlawson commented Nov 26, 2024

cardoso commented Nov 26, 2024

nolanlawson commented Nov 26, 2024

nolanlawson commented Nov 26, 2024

nolanlawson commented Nov 26, 2024

nolanlawson commented Nov 26, 2024

cardoso commented Nov 26, 2024

nolanlawson commented Nov 26, 2024

cardoso commented Nov 20, 2024 •

edited

Loading

nolanlawson commented Nov 26, 2024 •

edited

Loading