Migrate to pnpm #1123

jeremywiebe · 2025-02-28T22:48:37Z

Summary:

This PR migrates this repo to pnpm. After the initial import, there were several changes I had to make to get all the scripts working. Many of these were the result of yarn resolving modules through the workspace root's node_modules whereas pnpm puts a node_modules dir in each package's dir.

Note: the cache priming action is failing because it uses the action definition from the base branch (ie. main) which still references yarn, while the branch it's trying to merge has a package.json that has a packageManager entry that specifies pnpm. This should be resolved once we land this branch.

Changes include:

Use workspace:* version for all in-repo dependencies
Prevent usage of package managers other than pnpm and pin pnpm to v10.0.0
Approve packages that have post-install scripts (pnpm doesn't run a script's post-install script unless it's approved) #security
Move all scripts to use pnpm instead of yarn
Fix globbing in check-type-definitions.js so that pnpm build:types works
Fix globbing in pre-publish-check-ci.js
ESLint: ignore generated docs/ directory
Add dependencies to eslint-plugin-khan that used to be available implicitly by yarn
ESLint: allow typedoc.config.js to be CJS
Migrate typedoc config to be a js file so we can just glob the packages to build docs for

Issue: FEI-6300

Test plan:

I ran each of the scripts in package.json including the publish:ci (which I temporarily modified to not do the last step which acctually publishes).

changeset-bot · 2025-02-28T22:50:57Z

🦋 Changeset detected

Latest commit: f75f2b2

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jeremywiebe · 2025-03-01T01:16:29Z

packages/wonder-stuff-ci/package.json

@@ -17,16 +17,16 @@
    "main": "dist/index.js",
    "types": "dist/index.d.ts",
    "scripts": {
-        "test": "bash -c 'yarn --silent --cwd \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"
+        "test": "bash -c 'pnpm --silent --dir \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"


Despite some work, I cannot get this to work. It runs, but does not run any tests.

I'm inclined to remove these test scripts from each of these packages. I don't know that anyone uses them and running tests from the root is quite easy (including the ability to filter to a single package by running pnpm test packages/wonder-stuff-ci).

You could try changing to pnpm run instead of just pnpm. I suspect that's what's missing.

Suggested change

"test": "bash -c 'pnpm --silent --dir \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"

"test": "bash -c 'pnpm run --silent --dir \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"

However, you can remove them. They are conveniences for running tests from within a package when editing that package because typing the package name every time is annoying, as-is scrolling through terminal history when working on stuff and running different commands.

I added these to help me dev faster, and they did. Would love to get them working, but not a big deal right now.

I've added it. Still doesn't run just the tests in the current package. I'm going to leave these here for now.

.eslintignore

jeremywiebe · 2025-03-17T22:05:36Z

package.json

-    "node-gyp": "^10.0.0"
+  "pnpm": {
+    "overrides": {
+      "a": "we need to pin wide-align/string-width because v5 & up are ESM only",


pnpm was complaining about the // resolutions when I migrated to overrides. Using an alphanumeric package name for the comment appears to work but feels pretty gross.

pnpm does support a package.json that includes comments (package.json5) but from what I've read not all other JS tooling does so that feels like a deal-breaker.

Do we still need these overrides at all?

Yeah, I dropped comments in here in repos I have migrated. I don't know of a good option for documenting this stuff that won't get out of sync. Just delete them?

I'm leaning that way too! Git should have details as to why the line was added even if it takes a bit of digging.

Could have a separate section for commenting that is custom of packageName -> comment and then have a test that verifies there's an override per comment and vice versa? Seems like a lot of work for now...so still...just delete them?

I'm leaning that way too! Git should have details as to why the line was added even if it takes a bit of digging.

This is harder than you might think and generally, doesn't happen unless someone absolutely needs to know. We have had this issue in the past quite a bit (why was this resolution there? Let me just get rid of it. Oh I can fix this error...) wasting lots of time when a comment right there would've said "we tried all that, just do this...see this ticket for info" and avoided the time-waste. Finding the right commit in Git can be a pain sometimes as code changes and moves around. Having the comments colocated was super useful and time-saving.

Ok, I brought it back with a pattern I think works. Install works which I think is the only place that'd parse that section of package.json. I do wonder if our current deps need that override, but I'm paranoid to remove it and how to fully verify.

…s to use workspace)

….0.0

…es to build docs for

…icitly by yarn

build-settings/check-type-definitions.ts

codecov · 2025-03-20T01:10:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.85%. Comparing base (97e6d36) to head (f75f2b2).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1123   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files          97       97           
  Lines        1390     1390           
  Branches      356      357    +1     
=======================================
  Hits         1388     1388           
  Misses          1        1           
  Partials        1        1

Files with missing lines	Coverage Δ
...der-stuff-render-server/src/requests-from-cache.ts	`100.00% <ø> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97e6d36...f75f2b2. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jeremywiebe · 2025-03-20T01:12:04Z

packages/eslint-plugin-khan/package.json

+  "dependencies": {
+    "ancesdir": "^5.0.1",
+    "@babel/types": "^7.23.0",
+    "@typescript-eslint/utils": "8.17.0",
+    "checksync": "^5.0.5"
+  },


These were moved up from below devDependencies, but also added the ancesdir dep. It used to work with yarn but pnpm lays out the node_modules dir differently and so we no longer see another package's (or root's) dependencies and we need to be explicit.

jeremywiebe · 2025-03-20T01:39:36Z

The Lint workflow is failing because the compressed-size-action is broken when the current and base branch use different package managers. This is a temporary issue and I don't know that I want to work through upstreaming a fix for this.

Unless folks disapprove, I plan to land this PR without addressing that issue. Future PRs won't have to worry about that because they won't be switching between package managers.

khan-actions-bot · 2025-03-20T01:39:53Z

Gerald

Required Reviewers

@Khan/frontend-infra for changes to .eslintrc.js, .gitattributes, CONTRIBUTING.md, package.json, pnpm-lock.yaml, pnpm-workspace.yaml, typedoc.config.js, .changeset/modern-actors-compete.md, .changeset/smart-beds-own.md, build-settings/check-type-definitions.ts, utils/pre-publish-check-ci.js, .github/workflows/node-ci-main.yml, .github/workflows/node-ci.yml, .github/workflows/release.yml, packages/eslint-config-khan/README.md, packages/eslint-config-khan/package.json, packages/eslint-plugin-khan/package.json, packages/wonder-stuff-ci/package.json, packages/wonder-stuff-core/package.json, packages/wonder-stuff-i18n/package.json, packages/wonder-stuff-render-environment-jsdom/package.json, packages/wonder-stuff-render-server/package.json, packages/wonder-stuff-sentry/package.json, packages/wonder-stuff-server/package.json, packages/wonder-stuff-testing/package.json, packages/eslint-plugin-khan/demo/README.md, packages/eslint-plugin-khan/demo/package.json, packages/wonder-stuff-render-environment-jsdom/examples/README.md, packages/wonder-stuff-render-server/examples/README.md, packages/wonder-stuff-render-server/src/requests-from-cache.ts, packages/wonder-stuff-i18n/src/bin/all-i18n-strings.ts, packages/wonder-stuff-i18n/src/bin/gen-potfile.ts

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

somewhatabstract

Thanks for tackling this. I think there are some edits needed (see inline comments), but I'll leave that to your discretion.

somewhatabstract · 2025-03-20T17:31:51Z

package.json

+    "nochangeset": "pnpm changeset add --empty",
+    "add:devdepbysha": "bash -c 'pnpm add --workspace-root --save-dev \"git+https://[email protected]/Khan/$0.git#$1\"'"
+  },
+  "packageManager": "[email protected]+sha512.b8fef5494bd3fe4cbd4edabd0745df2ee5be3e4b0b8b08fa643aa3e4c6702ccc0f00d68fa8a8c9858a735a0032485a44990ed2810526c875e416f001b17df12b"


question: Why pin to 10.0.0? Does the latest version not work here?

This is the exact value that we pin pnpm to in other projects. I think if we want to move to a newer one that'd be a good thing to do in a separate PR (and perhaps just churn through all repos we've migrated to pnpm in one shot).

Yeah, I know, but just copying it seems...I don't know, it feels like an anti-pattern (I did because everyone else did it). I don't know that it's really useful. What's the reason for pinning it for everything. The tooling (corepack, etc.) (at least for me with my different repos) handles the version changes seamlessly.

I don't think propagating a pinned version is necessary.

One reason is that pinning to a version with a hash like this gives us some protection against a supply chain attack. I would agree that there might not be much use just for that.

Happy to move this to be the most recent version. I do think it's valuable to do it the same way across our Khan Academy repos though.

Yes, pinning for sure. I think that it's demanded (all the repos I updated rely on that), I'm questioning pinning to the same version as everything else.

I do think it's valuable to do it the same way across our Khan Academy repos though.

Why? What's the value?

Ok, had a chat with John and I've changed my mind. I'm going to just change this to be [email protected] (latest - as you suggest). We can change that in a future PR if we want.

somewhatabstract · 2025-03-20T17:32:41Z

package.json

-    "node-gyp": "^10.0.0"
+  "pnpm": {
+    "overrides": {
+      "a": "we need to pin wide-align/string-width because v5 & up are ESM only",


Yeah, I dropped comments in here in repos I have migrated. I don't know of a good option for documenting this stuff that won't get out of sync. Just delete them?

.gitignore

build-settings/check-type-definitions.ts

somewhatabstract · 2025-03-20T19:58:39Z

packages/wonder-stuff-ci/package.json

@@ -17,16 +17,16 @@
    "main": "dist/index.js",
    "types": "dist/index.d.ts",
    "scripts": {
-        "test": "bash -c 'yarn --silent --cwd \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"
+        "test": "bash -c 'pnpm --silent --dir \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"


You could try changing to pnpm run instead of just pnpm. I suspect that's what's missing.

Suggested change

"test": "bash -c 'pnpm --silent --dir \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"

"test": "bash -c 'pnpm run --silent --dir \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"

However, you can remove them. They are conveniences for running tests from within a package when editing that package because typing the package name every time is annoying, as-is scrolling through terminal history when working on stuff and running different commands.

I added these to help me dev faster, and they did. Would love to get them working, but not a big deal right now.

packages/wonder-stuff-sentry/package.json

packages/wonder-stuff-server/package.json

utils/pre-publish-check-ci.js

somewhatabstract · 2025-03-20T20:09:26Z

packages/wonder-stuff-testing/package.json

note: This package needs deleting too but it's gnarly because of the wonder-stuff-ci dependency. So don't bother trying. I'll tackle it.

somewhatabstract · 2025-03-20T20:09:45Z

packages/wonder-stuff-render-server/examples/README.md

suggestion: You can delete this whole package. See ADR-825 and FEI-6388. I was trying to get it done before you had to migrate.

Co-authored-by: Jeff Yates <[email protected]>

jeremywiebe · 2025-03-20T22:19:12Z

I can't find the comment anymore, but in reply to the question about whether types build properly, they do. I build them on main and then on this branch and compared the sets of dist folders.

They were identical except for extremely small differences in the sourcemaps mappings field.

#1137) 🖍 _This is an audit!_ 🖍 ## Summary: I am blocked from landing #1123 because the `clean` step does not remove all `node_modules` directories. We use the `preactjs/compressed-size-action` to check build size changes in each PR. This action switches to the upstream branch after building on the PR branch. It then issues a 'clean' command (which we defined in our `package.json`). Finally, it installs packages and builds on the upstream branch. When a PR (#1123) switches package managers and leaves some `node_modules` folders in place after cleaning we have issues. In this case`yarn` ttempts to install over a `./node_modules` that was set up by `pnpm`, and [fails](https://github.com/Khan/wonder-stuff/actions/runs/13980489827/job/39144501346?pr=1123) (see screenshot below): <img width="1196" alt="image" src="https://github.com/user-attachments/assets/e47dd396-deb8-4a88-8a3c-6eec23f9d369" /> Issue: --none-- ## Test plan: I ran the following steps which simulate what's happening in the Check Builds step of PR #1123: 1. `git co jer/pnpm` 2. `find . -name node_modules -prune -exec rm -rf {} \;` # make sure _all_ node_modules are deleted 3. `pnpm install` 4. `git co jer/fix-cleanup` 5. `pnpm clean` # yes, using `pnpm` as that's how the `preactjs/compressed-size-action` does it 6. `yarn install` # this is the step that fails in the GitHub Action Success. Author: jeremywiebe Auditors: somewhatabstract Required Reviewers: Approved By: somewhatabstract Checks: ✅ 14 checks were successful, ⏭️ 3 checks have been skipped Pull Request URL: #1137

github-actions · 2025-03-21T21:51:52Z

Size Change: 0 B

Total Size: 4.56 kB

ℹ️ View Unchanged

Filename	Size
`packages/wonder-stuff-core/dist/browser/es/index.js`	1.85 kB
`packages/wonder-stuff-sentry/dist/browser/es/index.js`	1.59 kB
`packages/wonder-stuff-testing/dist/browser/es/index.js`	1.12 kB

_{compressed-size-action}

jeremywiebe self-assigned this Feb 28, 2025

jeremywiebe commented Mar 1, 2025

View reviewed changes

jeremywiebe force-pushed the jer/pnpm branch from e15dd2f to 28ffcad Compare March 17, 2025 21:38

jeremywiebe commented Mar 17, 2025

View reviewed changes

.eslintignore Outdated Show resolved Hide resolved

jeremywiebe commented Mar 17, 2025

View reviewed changes

jeremywiebe added 18 commits March 19, 2025 18:02

Initial import into pnpm (needed to update build settings package ref…

2f3cfa7

…s to use workspace)

Use workspace:* version for all in-repo dependencies

b0ab325

Prevent usage of package managers other than pnpm and pin pnpm to v10…

c6311c8

….0.0

Fix workspace refs to be workspace:*

46a3ade

Delete remaining yarn.lock files

423e5c7

Approve packages that have post-install scripts

4632b10

Migrate typedoc config to be a js file so we can just glob the packag…

988f9b4

…es to build docs for

Move all scripts to use pnpm instead of yarn

b8d8b1f

Fix globbing in check-type-definitions so that pnpm build:types works

33483bc

Fix globbing in pre-publish-check-ci.js

6bca055

ESLint: ignore generated docs/ directory

8bdc337

Add dependencies to eslint-plugin-khan that used to be available impl…

77641e0

…icitly by yarn

ESLint: allow typedoc.config.js to be CJS

c1a7ed5

Restore missing piece of publish:ci script

8e4cb01

Update Github actions to use pnpm

6bb6436

Get rid of more yarn-isms

5ce32e9

Bump actions

403045b

Upgrade to shared-node-cache-v3 which supports pnpm

8ab35cf

jeremywiebe force-pushed the jer/pnpm branch from abab947 to 8ab35cf Compare March 20, 2025 01:06

Revert unnecessary change to .eslintignore

6f61daa

jeremywiebe commented Mar 20, 2025

View reviewed changes

build-settings/check-type-definitions.ts Outdated Show resolved Hide resolved

jeremywiebe commented Mar 20, 2025

View reviewed changes

docs(changeset):

1aff0f8

docs(changeset): Migrate packages to use pnpm internally

586a841

jeremywiebe marked this pull request as ready for review March 20, 2025 01:39

khan-actions-bot requested a review from a team March 20, 2025 01:39

jeremywiebe requested a review from somewhatabstract March 20, 2025 01:40

somewhatabstract approved these changes Mar 20, 2025

View reviewed changes

jeremywiebe and others added 8 commits March 20, 2025 14:21

Restore yarn-error.log line in .gitignore

3f7cbe6

Remove unneeded 'globstar: true' option on fglob.sync() call

0e0ea61

Remove 'overrides' comment entry

297342e

Add 'run' to scripts for in-package tests

d01ba74

Fix workspace refs to be workspace:*

4099e37

Update utils/pre-publish-check-ci.js

5931174

Co-authored-by: Jeff Yates <[email protected]>

Fix so we can use ancesdir in workspace

c48e2dd

Restore resolution override comment - I think this works

38b7a6d

Just use latest pnpm

9bd657a

jeremywiebe enabled auto-merge (squash) March 20, 2025 22:30

jeremywiebe disabled auto-merge March 20, 2025 22:30

jeremywiebe mentioned this pull request Mar 20, 2025

[🔥AUDIT🔥] Update 'cleanup' script to remove _all_ node_modules folders #1137

Merged

Merge branch 'main' into jer/pnpm

f75f2b2

jeremywiebe merged commit 82ec707 into main Mar 21, 2025
8 checks passed

jeremywiebe deleted the jer/pnpm branch March 21, 2025 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to pnpm #1123

Migrate to pnpm #1123

jeremywiebe commented Feb 28, 2025 •

edited

Loading

changeset-bot bot commented Feb 28, 2025 •

edited

Loading

jeremywiebe Mar 1, 2025

somewhatabstract Mar 20, 2025

jeremywiebe Mar 20, 2025

jeremywiebe Mar 17, 2025

somewhatabstract Mar 20, 2025

jeremywiebe Mar 20, 2025

somewhatabstract Mar 20, 2025

somewhatabstract Mar 20, 2025 •

edited

Loading

jeremywiebe Mar 20, 2025

codecov bot commented Mar 20, 2025 •

edited

Loading

jeremywiebe Mar 20, 2025

jeremywiebe commented Mar 20, 2025

khan-actions-bot commented Mar 20, 2025 •

edited

Loading

somewhatabstract left a comment •

edited

Loading

somewhatabstract Mar 20, 2025

jeremywiebe Mar 20, 2025

somewhatabstract Mar 20, 2025 •

edited

Loading

jeremywiebe Mar 20, 2025

somewhatabstract Mar 20, 2025 •

edited

Loading

jeremywiebe Mar 20, 2025 •

edited

Loading

somewhatabstract Mar 20, 2025

somewhatabstract Mar 20, 2025

somewhatabstract Mar 20, 2025

somewhatabstract Mar 20, 2025

jeremywiebe commented Mar 20, 2025

github-actions bot commented Mar 21, 2025

	"test": "bash -c 'pnpm --silent --dir \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] \|\| [[ ${@: -1} = bash ]]) && echo $PWD)'"
	"test": "bash -c 'pnpm run --silent --dir \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] \|\| [[ ${@: -1} = bash ]]) && echo $PWD)'"

Migrate to pnpm #1123

Migrate to pnpm #1123

Conversation

jeremywiebe commented Feb 28, 2025 • edited Loading

Summary:

Test plan:

changeset-bot bot commented Feb 28, 2025 • edited Loading

🦋 Changeset detected

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

somewhatabstract Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 20, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

jeremywiebe commented Mar 20, 2025

khan-actions-bot commented Mar 20, 2025 • edited Loading

Gerald

somewhatabstract left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somewhatabstract Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somewhatabstract Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

jeremywiebe Mar 20, 2025 • edited Loading

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

jeremywiebe commented Mar 20, 2025

github-actions bot commented Mar 21, 2025

jeremywiebe commented Feb 28, 2025 •

edited

Loading

changeset-bot bot commented Feb 28, 2025 •

edited

Loading

somewhatabstract Mar 20, 2025 •

edited

Loading

codecov bot commented Mar 20, 2025 •

edited

Loading

khan-actions-bot commented Mar 20, 2025 •

edited

Loading

somewhatabstract left a comment •

edited

Loading

somewhatabstract Mar 20, 2025 •

edited

Loading

somewhatabstract Mar 20, 2025 •

edited

Loading

jeremywiebe Mar 20, 2025 •

edited

Loading