chore: Drop `yarn2nix` and switch to `pnpm`. #358

dhess · 2022-05-25T18:30:17Z

This is a wide-ranging change in which we drop yarn2nix (and Yarn
itself) in favor using pnpm directly, with no Nix layer in-between
the pnpm lockfile and the packages we use for builds. This means
we're giving up on most of what Nix provides -- here we only use it to
ensure that everyone is using the same version of pnpm, sourced from
nipxkgs via our nix develop shell.

We also now run most CI on GitHub Actions, rather than Buildkite. See
the notes below for a detailed justification.

I'm doing this reluctantly, but yarn2nix is falling further and
further behind the Nodejs ecosystem, it always feels like we're just
one major package version bump away from everything breaking, and
there's no viable replacement on the horizon for yarn2nix that I can
see. Nix is not really buying us much in this repo to begin with, so
we don't lose that much with this change. Hopefully this leads to
better DX going forward overall.

Note that the README.md is heavily edited to replace all the yarn
commands with the equivalent pnpm commands (where they exist; we
have lost some functionality by dropping Yarn, see below), and I
wouldn't be surprised if there are some discrepancies in the
documentation vs the reality. Please file an issue if you notice any.

A few more items of note:

I have bumped our nixpkgs pin here, but only to get a more recent
version of pnpm. I have not bumped any Nodejs package versions.
pnpm is more strict about dependencies, so I've had to add a few
that Yarn was magically finding for us.
We've lost the nice wsrun watch and wsrun watch:storybook
commands that would run the entire project in watch (HMR) mode with
a single command. This is because wsrun is designed to work with
Yarn workspaces only, and doesn't work with pnpm workspaces. As far
as I can tell, there's no equivalent to wsrun for pnpm, which
means that if you want the run the frontend or our Storybook in HMR
mode and automatically pick up changes made to the other packages in
the repo upon which the frontend/Storybook depend, you'll need to open
multiple shells and run a pnpm watch command in each one,
simultaneously.
With this change, we build the frontend and test it (or at least,
will test it, once we have some tests to run!) on GitHub Actions, not
on Buildkite. The reasons for this are that while the Nix Buildkite
plugin is wonderful, and works much better than any Nix-related GitHub
Action that I've seen, there are no comparable Buildkite plugins for
installing a particular version of pnpm, caching pnpm builds
across multiple CI runs, or deploying to Chromatic. Therefore, if we
stuck with Buildkite CI for this repo, we would need to implement all
that functionality ourselves, probably via bespoke scripts. That's a
waste of time when there are existing vendor-provided GitHub Actions
which do all of those things for us. We still do have a few minor
checks & some Nix shell caching CI jobs on Buildkite, but most likely
those will eventually go away and be replaced by equivalent GitHub
Actions, so that we only need to look in one place for CI results.
On that note, we do not implement the push-to-Chromatic CI step
here, as I want to test just building the app before we get into
trying to build Storybook in CI. (However, I have been able to
successfully build and use Storybook in my local shell with these
changes.) Support for this is coming in a subsequent commit.
Re: Storybook, we have to do some ugly hacks to make it work
with pnpm. Many people have trouble with this and there are a litany
of issues, many with different workarounds that are sometimes
successful, sometimes not; see:

pnpm/pnpm#4268
storybookjs/storybook#13428
https://github.com/storybookjs/builder-vite/blob/782261437b3cb3e2f3d013cd37f1faa14863f205/packages/builder-vite/README.md#installation
storybookjs/builder-vite#55
storybookjs/builder-vite#237
https://github.com/IanVS/vite-storybook-pnpm-shame
https://github.com/jdk2pq/vite-storybook-pnpm-vue

Until Storybook fix their various issues, this looks like the proper
"fix", but it will require quite a bit of work, I suspect:

pnpm/pnpm#4268 (comment)

However, in the interest of time, I've chosen this option, which is
the least bad workaround I've seen that worked for me:

storybookjs/storybook#13428 (comment)

--

Fixes #333.

brprice · 2022-05-25T19:17:35Z

We do not implement the push-to-Chromatic CI step here, as I want to
test just building the app before we get into trying to build
Storybook in CI. (However, I have been able to successfully build and
use Storybook in my local shell with these changes.)
...
Speaking of Storybook, ...
in the interest of time, I've chosen this option, which is the least bad workaround I've seen that worked for me:

It also works for me:

pnpm install
cd packages/primer-components
pnpm storybook

opens a working local storybook. (I am using f19b872).

dhess · 2022-05-25T19:19:46Z

This is good to go, I think. I'll wait for @georgefst to rebase #339 on this before merging.

georgefst · 2022-05-26T09:16:14Z

It also works for me:
pnpm install
cd packages/primer-components
pnpm storybook
opens a working local storybook. (I am using f19b872).

Weirdly, I had to additionally run pnpm add -D @storybook/addon-docs in order for Storybook to successfully load.

dhess · 2022-05-26T09:25:03Z

It also works for me:
pnpm install
cd packages/primer-components
pnpm storybook
opens a working local storybook. (I am using f19b872).
Weirdly, I had to additionally run pnpm add -D @storybook/addon-docs in order for Storybook to successfully load.

Hmm, that should not have been necessary. I assume you're doing this from the nix develop shell? Did you recursively remove remove your node_modules directories that were leftover from Yarn?

georgefst · 2022-05-26T09:34:37Z

It also works for me:
pnpm install
cd packages/primer-components
pnpm storybook
opens a working local storybook. (I am using f19b872).
Weirdly, I had to additionally run pnpm add -D @storybook/addon-docs in order for Storybook to successfully load.
Hmm, that should not have been necessary. I assume you're doing this from the nix develop shell? Did you recursively remove remove your node_modules directories that were leftover from Yarn?

Yes.

Without adding that dependency, I got the following error:

$ pnpm storybook

> @hackworthltd/primer-components@0.2.0 storybook /Users/gthomas/projects/primer-app/packages/primer-components
> start-storybook -p 6006

info @storybook/react v6.5.5
info 
info => Loading presets
info => Using prebuilt manager
╭───────────────────────────────────────────────────╮
│                                                   │
│   Storybook 6.5.5 for React started               │
│   1.29 s for preview                              │
│                                                   │
│    Local:            http://localhost:6006/       │
│    On your network:  http://192.168.1.85:6006/    │
│                                                   │
╰───────────────────────────────────────────────────╯
10:12:47 [vite] Internal server error: Failed to resolve import "@storybook/addon-docs/preview.js" from "../../../../../../virtual:/@storybook/builder-vite/vite-app.js". Does the file exist?
  Plugin: vite:import-analysis
  File: /virtual:/@storybook/builder-vite/vite-app.js
  15 |  import * as config_1 from '@storybook/react/dist/esm/client/preview/config'
  16 |  import * as config_2 from '@storybook/addon-links/preview.js'
  17 |  import * as config_3 from '@storybook/addon-docs/preview.js'
     |                             ^
  18 |  import * as config_4 from '@storybook/addon-actions/preview.js'
  19 |  import * as config_5 from '@storybook/addon-backgrounds/preview.js'
      at formatError (/Users/gthomas/projects/primer-app/node_modules/.pnpm/vite@2.9.9/node_modules/vite/dist/node/chunks/dep-59dc6e00.js:38663:46)
      at TransformContext.error (/Users/gthomas/projects/primer-app/node_modules/.pnpm/vite@2.9.9/node_modules/vite/dist/node/chunks/dep-59dc6e00.js:38659:19)
      at normalizeUrl (/Users/gthomas/projects/primer-app/node_modules/.pnpm/vite@2.9.9/node_modules/vite/dist/node/chunks/dep-59dc6e00.js:56830:26)
      at async TransformContext.transform (/Users/gthomas/projects/primer-app/node_modules/.pnpm/vite@2.9.9/node_modules/vite/dist/node/chunks/dep-59dc6e00.js:56979:57)
      at async Object.transform (/Users/gthomas/projects/primer-app/node_modules/.pnpm/vite@2.9.9/node_modules/vite/dist/node/chunks/dep-59dc6e00.js:38900:30)
      at async doTransform (/Users/gthomas/projects/primer-app/node_modules/.pnpm/vite@2.9.9/node_modules/vite/dist/node/chunks/dep-59dc6e00.js:55857:29)

But now, I can't reproduce that, even after wiping node_modules again...

So I guess everything's fine? Slightly concerning though.

georgefst · 2022-05-26T09:35:42Z

I'm also finding that nix develop modifies flake.lock, which I wouldn't have expected on a fresh checkout:

$ git diff
diff --git a/flake.lock b/flake.lock
index 21d1b90..ed3bfad 100644
--- a/flake.lock
+++ b/flake.lock
@@ -234,6 +234,22 @@
         "type": "github"
       }
     },
+    "nixpkgs_4": {
+      "locked": {
+        "lastModified": 1653326962,
+        "narHash": "sha256-W8feCYqKTsMre4nAEpv5Kx1PVFC+hao/LwqtB2Wci/8=",
+        "owner": "NixOS",
+        "repo": "nixpkgs",
+        "rev": "41cc1d5d9584103be4108c1815c350e07c807036",
+        "type": "github"
+      },
+      "original": {
+        "owner": "NixOS",
+        "ref": "nixpkgs-unstable",
+        "repo": "nixpkgs",
+        "type": "github"
+      }
+    },
     "pre-commit-hooks-nix": {
       "inputs": {
         "flake-utils": [
@@ -272,10 +288,7 @@
     },
     "sops-nix": {
       "inputs": {
-        "nixpkgs": [
-          "hacknix",
-          "nixpkgs"
-        ],
+        "nixpkgs": "nixpkgs_4",
         "nixpkgs-21_11": "nixpkgs-21_11"
       },
       "locked": {

georgefst · 2022-05-26T09:45:17Z

I'm also finding that nix develop modifies flake.lock, which I wouldn't have expected on a fresh checkout:

Ok, this was fixed by updating hackworth-nix.

dhess · 2022-05-26T10:35:35Z

Hmm, I wouldn't expect this kind of strange interaction between hackworth-nix and our repos; that's allegedly one of the things that Flakes improve.

Anyway, I apologize for the trouble it caused.

This is a wide-ranging change in which we drop `yarn2nix` (and Yarn itself) in favor using `pnpm` directly, with no Nix layer in-between the `pnpm` lockfile and the packages we use for builds. This means we're giving up on most of what Nix provides -- here we only use it to ensure that everyone is using the same version of `pnpm`, sourced from `nipxkgs` via our `nix develop` shell. We also now run most CI on GitHub Actions, rather than Buildkite. See the notes below for a detailed justification. I'm doing this reluctantly, but `yarn2nix` is falling further and further behind the Nodejs ecosystem, it always feels like we're just one major package version bump away from everything breaking, and there's no viable replacement on the horizon for `yarn2nix` that I can see. Nix is not really buying us much in this repo to begin with, so we don't lose that much with this change. Hopefully this leads to better DX going forward overall. Note that the `README.md` is heavily edited to replace all the `yarn` commands with the equivalent `pnpm` commands (where they exist; we have lost some functionality by dropping Yarn, see below), and I wouldn't be surprised if there are some discrepancies in the documentation vs the reality. Please file an issue if you notice any. A few more items of note: * I have bumped our `nixpkgs` pin here, but only to get a more recent version of `pnpm`. I have not bumped any Nodejs package versions. * `pnpm` is more strict about dependencies, so I've had to add a few that Yarn was magically finding for us. * We've lost the nice `wsrun watch` and `wsrun watch:storybook` commands that would run the entire project in `watch` (HMR) mode with a single command. This is because `wsrun` is designed to work with Yarn workspaces only, and doesn't work with `pnpm` workspaces. As far as I can tell, there's no equivalent to `wsrun` for `pnpm`, which means that if you want the run the frontend or our Storybook in HMR mode and automatically pick up changes made to the other packages in the repo upon which the frontend/Storybook depend, you'll need to open multiple shells and run a `pnpm watch` command in each one, simultaneously. * With this change, we build the frontend and test it (or at least, will test it, once we have some tests to run!) on GitHub Actions, not on Buildkite. The reasons for this are that while the Nix Buildkite plugin is wonderful, and works much better than any Nix-related GitHub Action that I've seen, there are no comparable Buildkite plugins for installing a particular version of `pnpm`, caching `pnpm` builds across multiple CI runs, or deploying to Chromatic. Therefore, if we stuck with Buildkite CI for this repo, we would need to implement all that functionality ourselves, probably via bespoke scripts. That's a waste of time when there are existing vendor-provided GitHub Actions which do all of those things for us. We still do have a few minor checks & some Nix shell caching CI jobs on Buildkite, but most likely those will eventually go away and be replaced by equivalent GitHub Actions, so that we only need to look in one place for CI results. * On that note, we do not implement the push-to-Chromatic CI step here, as I want to test just building the app before we get into trying to build Storybook in CI. (However, I have been able to successfully build and use Storybook in my local shell with these changes.) Support for this is coming in a subsequent commit. * Speaking of Storybook, we have to do some ugly hacks to make it work with `pnpm`. Many people have trouble with this and there are a litany of issues, many with different workarounds that are sometimes successful, sometimes not; see: pnpm/pnpm#4268 storybookjs/storybook#13428 https://github.com/storybookjs/builder-vite/blob/782261437b3cb3e2f3d013cd37f1faa14863f205/packages/builder-vite/README.md#installation storybookjs/builder-vite#55 storybookjs/builder-vite#237 https://github.com/IanVS/vite-storybook-pnpm-shame https://github.com/jdk2pq/vite-storybook-pnpm-vue Until Storybook fix their various issues, this looks like the proper "fix", but it will require quite a bit of work, I suspect: pnpm/pnpm#4268 (comment) However, in the interest of time, I've chosen this option, which is the least bad workaround I've seen that worked for me: storybookjs/storybook#13428 (comment)

dhess force-pushed the dhess/pnpm branch 4 times, most recently from f71e16c to d0af783 Compare May 25, 2022 19:16

This was referenced May 25, 2022

feat: Add a tree-rendering component using React Flow #339

Merged

Investigate pnpm and Yarn 3 #333

Closed

Change src attribute for Node packages #210

Closed

prettier in nixpkgs is out of sync/broken #138

Open

CI should run eslint #187

Closed

dhess force-pushed the dhess/pnpm branch from d0af783 to 223f035 Compare May 25, 2022 21:24

dhess added 2 commits May 26, 2022 12:13

ci: Add a Deploy-to-Chromatic GitHub workflow that works with pnpm.

9b8d1ad

dhess force-pushed the dhess/pnpm branch from 223f035 to 9b8d1ad Compare May 26, 2022 11:24

dhess added the Ready to merge Ready to merge label May 26, 2022

ci: Update Mergify config for new checks post-pnpm switch.

3436200

dhess mentioned this pull request May 26, 2022

Change required jobs once #358 is merged #362

Closed

dhess merged commit 0c8362c into main May 26, 2022

dhess deleted the dhess/pnpm branch May 26, 2022 11:33

dhess mentioned this pull request May 26, 2022

Fix Mergify config #363

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Drop `yarn2nix` and switch to `pnpm`. #358

chore: Drop `yarn2nix` and switch to `pnpm`. #358

dhess commented May 25, 2022 •

edited

brprice commented May 25, 2022

dhess commented May 25, 2022

georgefst commented May 26, 2022

dhess commented May 26, 2022

georgefst commented May 26, 2022

georgefst commented May 26, 2022

georgefst commented May 26, 2022

dhess commented May 26, 2022

chore: Drop yarn2nix and switch to pnpm. #358

chore: Drop yarn2nix and switch to pnpm. #358

Conversation

dhess commented May 25, 2022 • edited

brprice commented May 25, 2022

dhess commented May 25, 2022

georgefst commented May 26, 2022

dhess commented May 26, 2022

georgefst commented May 26, 2022

georgefst commented May 26, 2022

georgefst commented May 26, 2022

dhess commented May 26, 2022

chore: Drop `yarn2nix` and switch to `pnpm`. #358

chore: Drop `yarn2nix` and switch to `pnpm`. #358

dhess commented May 25, 2022 •

edited