*: switch to pnpm #527

sjbarag · 2023-06-30T22:30:57Z

* // switch to pnpm

We're moving in this direction in other repos due to Bazel's rules_js — may as well be consistent.

Checklist

I have written or updated test for the changes I made
I have updated the README of the package I'm working in to reflect my changes
I have added or updated Storybook if appropriate for my changes

sjbarag · 2023-06-30T22:44:00Z

packages/design-tokens/package.json

@@ -13,9 +13,7 @@
  "types": "dist/web/tokens.d.ts",
  "scripts": {
    "build": "style-dictionary build",
-    "clean": "style-dictionary clean",
-    "prepare": "npm run clean && npm run build",


Something about the "automatically build after running pnpm install" behavior felt odd, so I removed it 😄

That's fair. I never could get the right feel for the correct life-cycle script here. I think I had "prepublishOnly" at one point but it may have been deprecated. This becomes a lot less critical with auto-publishing where we don't need lifecycle scripts to help us remember to "build" before we "publish".

sjbarag · 2023-06-30T22:44:40Z

packages/icons/package.json

    "build:typescript": "tsc",
    "build:generate:stripfill": "svgr --typescript --icon -d src/components svg/customFill",
    "build:generate:preserve": "svgr --typescript --icon --no-runtime-config -d src/components svg/preserveFill",
-    "build:watch": "tsc --watch",
+    "build:watch": "tsc --watch --preserveWatchOutput",


Without this, tsc --watch clears the terminal while other watch tasks don't and it looks weird!

sjbarag · 2023-06-30T22:45:18Z

packages/storybook-ui-components/package.json

+    "@cockroachlabs/icons": "workspace:../icons",
+    "@cockroachlabs/ui-components": "workspace:../ui-components",


hooray workspace: protocol: https://pnpm.io/workspaces#workspace-protocol-workspace 🎉

This is a huge improvement honestly. I haven't gotten there yet, but any idea how this will be handled when building storybook for deployment?

It shouuuuuuuld automatically link everything together :)

sjbarag · 2023-06-30T22:45:44Z

packages/ui-components/package.json

    "@popperjs/core": "^2.9.2",
-    "@types/recharts": "^1.8.23",
+    "lodash": "^4.17.21",


Accidentally-missing dependency (previously hoisted from another package)

sjbarag · 2023-06-30T22:45:54Z

packages/ui-components/package.json

    "@popperjs/core": "^2.9.2",
-    "@types/recharts": "^1.8.23",


This was never a runtime dependency — moved to dev dependencies

sjbarag · 2023-06-30T22:46:03Z

packages/ui-components/package.json

@@ -52,6 +52,8 @@
    "@types/node": "16.18.14",
    "@types/react": "17.0.53",
    "@types/react-lines-ellipsis": "0.15.1",
+    "@types/recharts": "^1.8.23",
+    "@types/testing-library__jest-dom": "^5.14.6",


Accidentally-missing dependency (previously provided by @testing-library/jest-dom as a transitive dependency)

sjbarag · 2023-06-30T22:46:12Z

packages/ui-components/package.json

@@ -68,6 +70,7 @@
    "jest-environment-jsdom": "29.4.3",
    "jest-runner-eslint": "1.1.0",
    "mini-css-extract-plugin": "^2.7.5",
+    "npm-run-all": "4.1.5",


Accidentally missing dependency (got hoisted from another package)

sjbarag · 2023-06-30T22:47:11Z

packages/ui-components/webpack.config.js

@@ -1,7 +1,6 @@
 /* eslint-disable @typescript-eslint/no-var-requires */
 /* globals module, __dirname */
 const path = require("path");
-const WebpackBar = require("webpackbar");


It got in the way of watch task reporting and spewed way too many lines. I can put it back if we want, but 🤷

Nah, this was something I was playing with way back in the beginning. It was only something to clean up the output of watching only ui-components. I agree it doesn't make sense in the context of file watching.

nathanstilwell

So this review looks good, but there's one big catch, and that is that I can't get auto publishing to work with this setup. Here's my wip branch for reference.

So auto depends on lerna to determine what changed and needs to publish. I tried adding lerna back and configuring for pnpm (that part might work correctly). But when I run auto I get an error in pnpm,

╔═══════════════════════════
║ ui on  switch-to-pnpm [⇡] via  v16.20.1
╚════  △ pnpm run release

> root@ release /Users/nathanstilwell/code/ui
> auto shipit

⚠  warning   lerna notice cli v7.1.1
lerna success found 5 packages

TypeError: Cannot read properties of undefined (reading 'reduce')
    at findPackages (/Users/nathanstilwell/code/ui/node_modules/.pnpm/[email protected]/node_modules/get-monorepo-packages/index.js:17:6)
    at Object.getPackages [as default] (/Users/nathanstilwell/code/ui/node_modules/.pnpm/[email protected]/node_modules/get-monorepo-packages/index.js:37:14)
    at getMonorepoPackage (/Users/nathanstilwell/code/ui/node_modules/.pnpm/@[email protected]_@[email protected][email protected]/node_modules/@auto-it/npm/src/index.ts:158:31)
    at /Users/nathanstilwell/code/ui/node_modules/.pnpm/@[email protected]_@[email protected][email protected]/node_modules/@auto-it/npm/src/index.ts:1098:30
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
 ELIFECYCLE  Command failed with exit code 1.

I did not dig in very far here. In my mind we have a couple of options to proceed forward,

Debug this error and submit a patch for pnpm or auto or whatever to resolve the issue.
Create our own plugin for auto.
Drop auto entirely and create a solution using only pnpm and Github Actions.

Happy to brainstorm further.

nathanstilwell · 2023-07-10T17:40:31Z

package.json

+    "start": "pnpm run --recursive --parallel start",
+    "test": "pnpm run --recursive test",
+    "build": "pnpm run --recursive build",


The output for these is actually really nice!

nathanstilwell · 2023-07-10T17:42:44Z

packages/design-tokens/package.json

@@ -13,9 +13,7 @@
  "types": "dist/web/tokens.d.ts",
  "scripts": {
    "build": "style-dictionary build",
-    "clean": "style-dictionary clean",
-    "prepare": "npm run clean && npm run build",


That's fair. I never could get the right feel for the correct life-cycle script here. I think I had "prepublishOnly" at one point but it may have been deprecated. This becomes a lot less critical with auto-publishing where we don't need lifecycle scripts to help us remember to "build" before we "publish".

nathanstilwell · 2023-07-10T17:45:03Z

packages/icons/package.json

@@ -8,15 +8,14 @@
  "main": "dist/index.js",
  "types": "dist/index.d.ts",
  "scripts": {
-    "build": "npm run build:generate:stripfill && npm run build:generate:preserve && npm run build:typescript",
+    "build": "run-s build:generate:stripfill build:generate:preserve build:typescript",


Ooo. What is run-s? Does that come with pnpm? If it's a dependency-less npm-run-all then I love it.

Unfortunately it's an alias that comes wioth npm-run-all. I don't think pnpm supports sequential script targets natively 🤔

nathanstilwell · 2023-07-10T17:47:23Z

packages/storybook-ui-components/package.json

+    "@cockroachlabs/icons": "workspace:../icons",
+    "@cockroachlabs/ui-components": "workspace:../ui-components",


This is a huge improvement honestly. I haven't gotten there yet, but any idea how this will be handled when building storybook for deployment?

nathanstilwell · 2023-07-10T17:51:01Z

packages/ui-components/webpack.config.js

@@ -1,7 +1,6 @@
 /* eslint-disable @typescript-eslint/no-var-requires */
 /* globals module, __dirname */
 const path = require("path");
-const WebpackBar = require("webpackbar");


Nah, this was something I was playing with way back in the beginning. It was only something to clean up the output of watching only ui-components. I agree it doesn't make sense in the context of file watching.

sjbarag · 2023-09-12T17:28:23Z

I thiiiink I've got auto working now! Had to upgrade to 11.x and lerna 7.x, but everything seems good. Note that auto shipit --dry-run doesn't work due to intuit/auto#2129. Something's just weird with --dry-run 🤷

nathanstilwell

I guess we just had to wait until Lerna and pnpm learned to play together. 😄

sjbarag force-pushed the switch-to-pnpm branch 2 times, most recently from aaeae2a to 43296b3 Compare June 30, 2023 22:40

sjbarag commented Jun 30, 2023

View reviewed changes

sjbarag marked this pull request as ready for review June 30, 2023 22:47

sjbarag requested a review from a team as a code owner June 30, 2023 22:47

sjbarag requested review from nathanstilwell and laurenbarker June 30, 2023 22:49

nathanstilwell reviewed Jul 10, 2023

View reviewed changes

sjbarag force-pushed the switch-to-pnpm branch from 1a5827e to 5b98eef Compare September 12, 2023 16:59

build: switch to pnpm

5d8ddd9

sjbarag force-pushed the switch-to-pnpm branch from 5b98eef to 5d8ddd9 Compare September 12, 2023 17:27

sjbarag requested a review from nathanstilwell September 12, 2023 17:28

nathanstilwell approved these changes Sep 12, 2023

View reviewed changes

sjbarag merged commit 559cdaa into cockroachdb:master Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: switch to pnpm #527

*: switch to pnpm #527

sjbarag commented Jun 30, 2023 •

edited

Loading

sjbarag Jun 30, 2023

nathanstilwell Jul 10, 2023

sjbarag Jun 30, 2023

sjbarag Jun 30, 2023

nathanstilwell Jul 10, 2023

sjbarag Jul 10, 2023

sjbarag Jun 30, 2023

sjbarag Jun 30, 2023

sjbarag Jun 30, 2023

sjbarag Jun 30, 2023

sjbarag Jun 30, 2023

nathanstilwell Jul 10, 2023

nathanstilwell left a comment

nathanstilwell Jul 10, 2023

nathanstilwell Jul 10, 2023

nathanstilwell Jul 10, 2023

sjbarag Jul 10, 2023

nathanstilwell Jul 10, 2023

nathanstilwell Jul 10, 2023

sjbarag commented Sep 12, 2023

nathanstilwell left a comment

		"@cockroachlabs/icons": "workspace:../icons",
		"@cockroachlabs/ui-components": "workspace:../ui-components",

*: switch to pnpm #527

*: switch to pnpm #527

Conversation

sjbarag commented Jun 30, 2023 • edited Loading

* // switch to pnpm

Checklist

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

nathanstilwell 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

sjbarag commented Sep 12, 2023

nathanstilwell left a comment

Choose a reason for hiding this comment

sjbarag commented Jun 30, 2023 •

edited

Loading