Try pnpm (take two) #38624

sarayourfriend · 2022-02-08T13:48:26Z

Description

I'll force push this onto try/pnpm in #37324 once this is working with CI so we can preserve the original PR discussion.

Testing Instructions

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
I've updated related schemas if appropriate.

sarayourfriend · 2022-02-08T13:56:25Z

The Pull request automation job is going to continue failing until the new action I've added is on trunk. Not sure there's a way around that for this initial PR.

sarayourfriend · 2022-02-08T14:01:30Z

Failing on installation due to some patches not applying correctly. Either we need to hoist the packages we're patching or I think we could also just change the patch diffs to point to the .pnpm prefix that pnpm uses. I'll experiment with both, first trying to reproduce the issue locally.

sarayourfriend · 2022-02-08T14:25:54Z

I think I need to bust the cache somehow 🤔 Seems like the cache calculation should include .npmrc too 😅

sarayourfriend · 2022-02-08T14:33:01Z

That worked!

Now there seem to be some TypeScript resolution issues. Cursory searching seems to indicate TypeScript should "just work" with pnpm but clearly there's some issue with our setup here.

I also can't reproduce the issue locally yet, so that's probably the first step.

Will report back anything I find 🙂

sarayourfriend · 2022-02-08T14:39:43Z

Aha! Looking at the first couple types that aren't being resolved it seems like they're indeclared development dependencies! pnpm is already doing it's job for us and pointing out assumptions we've made in the dependency chain! Marvelous 🤩

sarayourfriend · 2022-02-08T14:54:29Z

Had to squash changes to make rebasing easier when package-lock.json changes on trunk. Sorry if this makes the history and comments confusing!

sarayourfriend · 2022-02-08T14:59:04Z

The plugin-zip building script needs to be updated to use pnpm. npm cache verify doesn't need to happen anymore I don't believe as pnpm will automatically verify the cache when running pnpm install (maybe??). I'll do some more research on this and try to discover what the right way of doing that with pnpm is.

sarayourfriend · 2022-02-08T16:00:02Z

This will on some level be blocked by until this PR is merged for the compressed-size-check action: preactjs/compressed-size-action#68

sarayourfriend · 2022-02-08T20:03:07Z

Using pnpm to run the build script seems to have moved the progress on this PR backwards 😰 I need to put this down for now but will revisit it next week again (when there are more merge conflicts to resolve, I am sure!)

kevin940726

Thank you for starting this! Spotted some tiny errors.

Will be happy to try it out once this is ready 👍

kevin940726 · 2022-02-09T02:58:45Z

.github/workflows/create-block.yml

              with:
-                  node-version: ${{ matrix.node }}
-                  cache: npm
+                  node-version: ${{ matrix-node }}


I think this should still be matrix.node?

Whoops, yes it should be!

kevin940726 · 2022-02-09T02:59:36Z

.github/workflows/flaky-tests.yml

              with:
-                  node-version: 14


Better to keep this?

The default is set to 14. I only kept passing it to the action in places that were still using the matrix strategy for it.

kevin940726 · 2022-02-09T03:01:14Z

.github/action/setup-node-env/action.yml

+        - name: Install
+          if: inputs.install
+          shell: bash
+          run: "pnpm install ${{ inputs.frozen-lockfile && '--frozen-lockfile' || '' }}"


Looking at the doc, seems like we don't have to pass --frozen-lockfile to true in CI since it's the default.

Interesting. I don't really understand the "For CI: true" remark. How can it tell if it's in a CI environment? This isn't described anywhere in the documentation as far as I could tell... might have to dig into the code to find that out when I come back to this.

I do think you're right in questioning why we would need it to ever not install with a frozen-lockfile on CI though. Maybe this PR is the right time to investigate that? 🤔

I believe most programs depend on the env variable process.env.CI to exist in CI environment. Most CI platforms, including GH Actions, set this variable.

kevin940726 · 2022-02-09T03:05:45Z

.github/action/setup-node-env/action.yml

+        required: false
+        description: Optional working directory to run installation in.
+    frozen-lockfile:
+        default: false


Why do we want to default it to false?

I'm not sure! There were places that were doing just npm install rather than npm ci and I didn't want to change their behavior in this PR.

In bd1d499 I changed it to just do a regular frozen-lockfile install everywhere relying on the default behavior in CI you mentioned. I can't think of a reason why CI should need to use a non-frozen install anywhere anyway.

kevin940726 · 2022-02-09T03:06:58Z

.github/action/setup-node-env/action.yml

+        default: false
+        description: Whether to install dependencies using `--frozen-lockfile`.
+    node-version:
+        default: 14


I think this is nice that we don't have to repeat passing node-version in every workflow if it's just running the default version. We can just update this file when upgrading node to a major version.

I guess this could also be changed to node-version-file: '.nvmrc' so that https://github.com/WordPress/gutenberg/blob/trunk/.nvmrc is the only place to change the version.

https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#node-version-file

Nice find! That sounds like a good option 🎉 We'd need to keep around node-version still for the workflows that use a node version matrix but referring to the .nvmrc file for the default is 💯

.nvmrc plays an important role for branches where we apply fixes to older WordPress major releases like wp/5.1.

Okay in bd1d499 I tried to set it up to just use .nvmrc by default unless a specific node-version is passed in (to preserve the ability for workflows to use the node version matrix, if that's still necessary). Hopefully it works, I'm not too strong on GitHub action expressions but should only need a little tweaking if it doesn't work right off the bat.

package.json

packages/scripts/scripts/check-licenses.js

…t versions declared

sarayourfriend · 2022-02-12T14:47:21Z

I just realized I'd been fighting dependency resolution issues through hoisting that are meant to be resolved through cross-workspace-package dependencies... Turns out file is meant to be replaced with workspace.

pnpm/pnpm#3642 (comment)

See https://github.com/microsoft/rushstack/tree/master/eslint/eslint-patch README for explanation why this is needed in a monorepo setting.

sarayourfriend · 2022-02-12T15:40:08Z

Turns out eslint and stylelint are both very special and refuse to resolve plugin modules in a modern way. eslint has a widely used patch for this behavior but stylelint does not, so the latter requires us to hoist its dependencies.

It turns out the patch will only work for a top-level eslint configuration. The nested react-native-editor configuration won't be helped by the patch and requires hoisting of the eslint-plugin dependencies anyway. I removed the patch and added a lengthy explanation for why we're hoisting those packages.

In general I think it'd be a good idea to continue documenting hoists. I feel like we can avoid just hoisting everything (which has it's own set of problems anyway) and everything that is documented and hoisted should theoretically be able to eventually be removed with the right dependency declarations.

Of course, that's with the exception of stylelint, eslint and anything used by expo, because those tools all have their own non-modern module resolution strategies that cause problems with pnpms strictness.

Consumers of the wp-scripts package outside the monorepo will not be effected by this as the dependencies will install at the top level of their repositories (unless they're also using a monorepo, I suppose, but then only if they use pnpm will they run into it).

Anyway, it's kind of a mess but unavoidable at this point as far as I can tell.

See mysticatea/npm-run-all#201

sarayourfriend · 2022-02-13T17:47:47Z

Regarding the current state of the unit tests: I think they might be flagging a potentially problematic (not generally, just for pnpm's strictness) assumption in the WebpackExtractDependenciesPlugin.

The fact that the e2e tests are passing consistently is exciting though!

sarayourfriend · 2022-09-20T00:19:08Z

Closing this draft. If anyone wants to pick this up once React Native is supported by pnpm then you'll have some starting points here though I would be things have changed in the mean time. The new npm versions might not make this change worthwhile anyway.

sarayourfriend force-pushed the try/pnpm-fresh branch 2 times, most recently from 9de3efb to 96edaf8 Compare February 8, 2022 13:55

sarayourfriend force-pushed the try/pnpm-fresh branch 2 times, most recently from 9858629 to ee56705 Compare February 8, 2022 14:53

sarayourfriend mentioned this pull request Feb 8, 2022

Try pnpm #37324

Closed

kevin940726 reviewed Feb 9, 2022

View reviewed changes

gziolo added [Type] Build Tooling Issues or PRs related to build tooling Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts labels Feb 12, 2022

sarayourfriend added 13 commits February 12, 2022 08:48

Run pnpm import and squash other changes to ease rebasing

e8eefdf

Remove package-lock.json linting

1683f5b

Declare dependency on csstype

d1cc228

Fix wonky @types/react resolution due to react-native having differen…

5fdf1e2

…t versions declared

Try fixing react type resolution with hoisting

482c2a7

Try a combination of hositing and tsconfig paths

815175f

Add explicit dev dependency on @types/node

aef8dc2

Add explicit jest-circus dependency

f41a70a

Remove lockfile linting step

9d69f1b

Modify performance test script for pnpm

07d7411

Use pnpm list to list installed packages

d00f798

Use pnpm to install and build plugin zip

199de59

Fix running native package scripts

6039e83

sarayourfriend added 4 commits February 12, 2022 08:48

Fix build script

4951764

Try hoisting within a package

202965e

Improve setup node action and use better defaults

bd1d499

Correct documentation/output

618ae4f

sarayourfriend force-pushed the try/pnpm-fresh branch from 15677ce to 618ae4f Compare February 12, 2022 14:09

sarayourfriend added 3 commits February 12, 2022 09:17

Try hositing inside the package that actually uses it

f53d8d1

Try shamefully-hoist

57b64dd

Fix workspace package references

7a351dc

sarayourfriend added 3 commits February 12, 2022 10:07

Try using link: for workspace root dependencies

cccb4df

pnpm/pnpm#3642 (comment)

Patch eslint module resolution

b742db6

See https://github.com/microsoft/rushstack/tree/master/eslint/eslint-patch README for explanation why this is needed in a monorepo setting.

Hoist stylelint dependencies

097bc58

sarayourfriend added 4 commits February 12, 2022 11:55

Break down and just hoist stuff

97bbb1a

Hoist namespaced plugins

359fb8d

Try workaround for npm-run-all not supporting pnpm

fd010de

See mysticatea/npm-run-all#201

Add missing top-level dev dependency

e5066de

sarayourfriend closed this Sep 20, 2022

gziolo deleted the try/pnpm-fresh branch September 20, 2022 04:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try pnpm (take two) #38624

Try pnpm (take two) #38624

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022 •

edited

Loading

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

kevin940726 left a comment

kevin940726 Feb 9, 2022

sarayourfriend Feb 9, 2022

kevin940726 Feb 9, 2022

sarayourfriend Feb 9, 2022

kevin940726 Feb 9, 2022

sarayourfriend Feb 9, 2022

kevin940726 Feb 9, 2022

kevin940726 Feb 9, 2022

sarayourfriend Feb 9, 2022

sarayourfriend Feb 12, 2022

kevin940726 Feb 9, 2022

ocean90 Feb 10, 2022

sarayourfriend Feb 10, 2022

gziolo Feb 12, 2022

sarayourfriend Feb 12, 2022

sarayourfriend commented Feb 12, 2022

sarayourfriend commented Feb 12, 2022 •

edited

Loading

sarayourfriend commented Feb 13, 2022 •

edited

Loading

sarayourfriend commented Sep 20, 2022

Try pnpm (take two) #38624

Try pnpm (take two) #38624

Conversation

sarayourfriend commented Feb 8, 2022

Description

Testing Instructions

Screenshots

Types of changes

Checklist:

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022 • edited Loading

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

sarayourfriend commented Feb 8, 2022

kevin940726 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend commented Feb 12, 2022

sarayourfriend commented Feb 12, 2022 • edited Loading

sarayourfriend commented Feb 13, 2022 • edited Loading

sarayourfriend commented Sep 20, 2022

sarayourfriend commented Feb 8, 2022 •

edited

Loading

sarayourfriend commented Feb 12, 2022 •

edited

Loading

sarayourfriend commented Feb 13, 2022 •

edited

Loading