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

cardoso · 2024-11-21T10:13:32Z

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

 ```
-yarn local
+pnpm local


Currently running into this issue: webdriverio/webdriverio#13787

Currently worked around it by adding tsx as devDependency in `@lwc/integration-tests".

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.

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

nolanlawson

Thank you for looking into this! I plan to give this a deeper look, but this week is crunch week so we're a bit time-strapped. 🙇

For the perf benchmark tests, don't worry about running all of them. If you'd like you can also set BENCHMARK_SAMPLE_SIZE=1 BENCHMARK_TIMEOUT=0 and it should run pretty fast.

cardoso · 2024-11-22T01:26:44Z

It seems changes are needed at least in these two scripts as well:

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

Lines 75 to 77 in f602082

    
           // The root must be whatever dir contains your `node_modules`, so that Tachometer can resolve bare npm imports 
        
           // See: https://github.com/google/tachometer/issues/244#issuecomment-2267191898 
        
           root: path.relative(path.dirname(htmlFilename), packageRootDir),

lwc/packages/@lwc/perf-benchmarks/scripts/fix-deps.sh

Lines 8 to 21 in f602082

    
           # Create symlinks in the local node_modules directory to point to the root ones. 
        
           # This is a bit bizarre, but it's very tricky to get Tachometer to allow bare npm-style imports in 
        
           # a monorepo project. We could potentially get rid of this script by doing relative imports instead. 
        
           # See: https://github.com/google/tachometer/issues/215#issuecomment-2267194394 
        
           set -e 
        
           mkdir -p ./node_modules/@lwc 
        
           for pkg in @lwc/engine-dom @lwc/engine-server @lwc/perf-benchmarks-components @lwc/ssr-runtime @lwc/synthetic-shadow; do 
        
             if [ ! -L "./node_modules/$pkg" ]; then 
        
               ln -s "../../../../../packages/$pkg" "./node_modules/$pkg" 
        
             fi 
        
           done

Currently observing these errors in the console:

Uncaught (in promise) TypeError: class CardComponent extends LightningElement {
  constructor(...args) {
    super(...args);
    this.title = void 0;
    // Note: This is only to give volume to the rehydration process.
    this.rows = void 0;
  }
  /*LWC compiler v8.10.1*/
} is not a valid component, or does not extends LightningElement from "lwc". You probably forgot to add the extend clause on the class declaration.
    at getComponentInternalDef (index.js:3972:13)
    at createVM (index.js:6905:14)
    at renderComponent (render-component.ts:72:5)
    at table-hydrate-1k.benchmark.js:103:21
    at table-hydrate-1k.benchmark.js:51:45
    at Array.map (<anonymous>)
    at runBenchmark (table-hydrate-1k.benchmark.js:51:29)

nolanlawson · 2024-11-22T22:53:54Z

I think this means that Rollup is bundling incorrectly and we're ending up with two copies of lwc. I'll take a look.

nolanlawson · 2024-11-22T23:02:46Z

/nucleus test

nolanlawson · 2024-11-22T23:06:13Z

/nucleus test

.package.json.swp

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

scripts/tasks/generate-license-files.js

 async function findLicenseText(depName) {
    // Iterate through possible names for the license file
    const names = ['LICENSE', 'LICENSE.md', 'LICENSE.txt'];

    // We would use require.resolve, but 1) that doesn't work if the module lacks a "main" in its `package.json`,
    // and 2) it gives us a deep `./path/to/index.js` which makes it harder to find a top-level LICENSE file. So
    // just assume that our deps are hoisted to the top-level `node_modules`.
-    const resolvedDepPath = path.join(process.cwd(), 'node_modules', depName);
+    const resolvedDepPath = list.find((dep) => dep.name === depName).paths[0];


Why does generate-license-files.js need to be updated?

The dependencies are not hoisted to the top-level anymore. Transitive dependencies like entities from '@parse5/tools` are also not hoisted to the package which depends on it. This is the only reasonable way to accomplish this.

Refs:

https://pnpm.io/symlinked-node-modules-structure

https://pnpm.io/how-peers-are-resolved

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

/nucleus test

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

OK yeah it looks like our fix-deps.sh script which is a hack to work around google/tachometer#215 really does not play nice with pnpm's node_modules system. When I run a tachometer test with --manual (e.g. ../../../node_modules/.bin/tach --config ./dist/__benchmarks__/engine-dom/benchmark-expression/expression.tachometer.json --manual from the perf-benchmarks directory), I can see that we are loading two copies of engine-dom:

http://127.0.0.1:8080/node_modules/@lwc/engine-dom/dist/index.js
http://127.0.0.1:8080/node_modules/@lwc/perf-benchmarks-components/node_modules/@lwc/engine-dom/dist/index.js

This has always been a nasty hacky, and now it's coming back to bite us. I may need to get clever here.

nolanlawson · 2024-11-22T23:44:39Z

I mean, at this point I would be happy to switch to npm or modern yarn instead. I'm not married to pnpm especially if it's going to give us headaches like this. I just wanted us to get off of yarn v1 😅

cardoso · 2024-11-23T00:08:14Z

@nolanlawson we could always use shamefully-hoist and avoid most changes here. The reason I didn't use it from the get-go was to try and get the full benefits of pnpm (especially avoiding the phantom dependencies).

Thanks for helping with the PR. I'll see if I can fix the issue with the benchmarks.

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

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

cardoso added 2 commits November 21, 2024 16:49

fix(perf-benchmarks): support both yarn and pnpm

122832d

fix: test:performance

850c264

nolanlawson reviewed Nov 21, 2024

View reviewed changes

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

nolanlawson reviewed Nov 21, 2024

View reviewed changes

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

nolanlawson reviewed Nov 21, 2024

View reviewed changes

cardoso added 3 commits November 21, 2024 21:08

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

eeb147e

fix: rollup build

ce0bd90

fix: add pnpm-lock.yaml to .prettierignore

f602082

chore: add instructions for Volta+pnpm

6916891

nolanlawson mentioned this pull request Nov 22, 2024

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

Closed

chore: fix github ci

e846524

nolanlawson mentioned this pull request Nov 22, 2024

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

Closed

nolanlawson reviewed Nov 22, 2024

View reviewed changes

.package.json.swp Outdated Show resolved Hide resolved

nolanlawson added 2 commits November 22, 2024 15:12

chore: sort overrides

189522b

fix: yarn vs pnpm in benchmark script

c8e5fbb

nolanlawson reviewed Nov 22, 2024

View reviewed changes

nolanlawson mentioned this pull request Nov 22, 2024

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

Closed

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

cardoso Nov 21, 2024

cardoso Nov 21, 2024

nolanlawson left a comment

cardoso commented Nov 21, 2024

nolanlawson left a comment

cardoso commented Nov 22, 2024

nolanlawson commented Nov 22, 2024

nolanlawson commented Nov 22, 2024

nolanlawson commented Nov 22, 2024

nolanlawson Nov 22, 2024

cardoso Nov 23, 2024

nolanlawson commented Nov 22, 2024

nolanlawson commented Nov 22, 2024

nolanlawson commented Nov 22, 2024

cardoso commented Nov 23, 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

Are you sure you want to change the base?

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

cardoso Nov 21, 2024

Choose a reason for hiding this comment

cardoso Nov 21, 2024

Choose a reason for hiding this comment

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

cardoso commented Nov 22, 2024

nolanlawson commented Nov 22, 2024

nolanlawson commented Nov 22, 2024

nolanlawson commented Nov 22, 2024

nolanlawson Nov 22, 2024

Choose a reason for hiding this comment

cardoso Nov 23, 2024

Choose a reason for hiding this comment

nolanlawson commented Nov 22, 2024

nolanlawson commented Nov 22, 2024

nolanlawson commented Nov 22, 2024

cardoso commented Nov 23, 2024

cardoso commented Nov 20, 2024 •

edited

Loading