Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .changeset/upgrade-pnpm-10.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
"@khanacademy/kas": patch
"@khanacademy/keypad-context": patch
"@khanacademy/kmath": patch
"@khanacademy/math-input": patch
"@khanacademy/perseus": patch
"@khanacademy/perseus-core": patch
"@khanacademy/perseus-editor": patch
"@khanacademy/perseus-linter": patch
"@khanacademy/perseus-score": patch
"@khanacademy/perseus-utils": patch
"@khanacademy/pure-markdown": patch
"@khanacademy/simple-markdown": patch
---

Upgrade pnpm from 10.22.0 to 10.32.1.
2 changes: 2 additions & 0 deletions knip.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ const config: KnipConfig = {
// `swcrc.jsc.experimental.plugins.push(["swc_mut_cjs_exports", {}]);`
// (hence, not imported).
"swc_mut_cjs_exports",
// @swc/helpers is referenced via externalHelpers in .swcrc, not imported directly.
"@swc/helpers",
],
// Scripts we use in `package.json`
ignoreBinaries: [
Expand Down
6 changes: 4 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"@storybook/test": "^8.6.15",
"@swc-node/register": "^1.10.10",
"@swc/core": "1.11.13",
"@swc/helpers": "^0.5.17",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need @swc/helpers? Is this related to the pnpm update?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was added due to some failing CI checks. Looks like the previous pnpm version was less explicit about things and this was being hoisted up before (or that's how Claude explained it). It fixed the related error (the checks couldn't find this).

Also, as I was bringing back Dependabot, Jeremy and I discussed just doing an update at the same time.

Copy link
Copy Markdown
Contributor Author

@Myranae Myranae Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the errors this fixed (link to the related check)

check related to cws/helpers

"@swc/jest": "^0.2.37",
"@testing-library/dom": "^10.3.1",
"@testing-library/jest-dom": "^6.4.6",
Expand Down Expand Up @@ -140,12 +141,13 @@
"nyc": {
"report-dir": "coverage/cypress/"
},
"packageManager": "pnpm@10.22.0+sha512.bf049efe995b28f527fd2b41ae0474ce29186f7edcb3bf545087bd61fbbebb2bf75362d1307fda09c2d288e1e499787ac12d4fcb617a974718a6051f2eee741c",
"packageManager": "pnpm@10.32.1+sha512.a706938f0e89ac1456b6563eab4edf1d1faf3368d1191fc5c59790e96dc918e4456ab2e67d613de1043d2e8c81f87303e6b40d4ffeca9df15ef1ad567348f2be",
"pnpm": {
"onlyBuiltDependencies": [
"cypress@13.6.5",
"esbuild@0.24.0",
"@swc/core@1.11.13"
"@swc/core@1.11.13",
"mathquill"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: mathquill in pnpm.onlyBuiltDependencies is the only entry without a version pin — the other entries all use name@version format (e.g. cypress@13.6.5). Without a version pin, any version of mathquill is allowed to run build scripts. Consider using mathquill@1.0.3 for consistency, though note this may not match correctly since mathquill is resolved from a GitHub tarball rather than a registry version.

Extended reasoning...

Pattern inconsistency in onlyBuiltDependencies

The pnpm.onlyBuiltDependencies array in package.json controls which packages are permitted to run build scripts (install hooks). All three existing entries use the name@version format to restrict this permission to a specific version:

  • cypress@13.6.5
  • esbuild@0.24.0
  • @swc/core@1.11.13

The newly added mathquill entry breaks this pattern by omitting a version pin. Per pnpm documentation, an unversioned entry means any version of that package would be allowed to run build scripts, which is less restrictive than the pinned approach used for every other entry.

Step-by-step proof

  1. Look at package.json line 147-150: the onlyBuiltDependencies array has four entries.
  2. The first three entries all use name@version format.
  3. The fourth entry, mathquill, has no version qualifier.
  4. In the lockfile, mathquill resolves to version 1.0.3 from github:Khan/mathquill#v1.0.3 (commit c9e4329b0bc5d9b4c21d765b5768e4e7693515b3).
  5. If someone were to change the mathquill specifier to a different version or fork, the unpinned entry would still allow that new version to run build scripts without any explicit approval.

Practical impact and caveats

The real-world risk here is minimal for several reasons: (1) mathquill is sourced from Khan's own GitHub repository, (2) it's pinned to a specific git tag and commit hash in the lockfile, and (3) any version change requires an explicit lockfile update that would go through code review.

Additionally, there's a practical question about whether mathquill@1.0.3 would even work correctly as a matcher for a GitHub-sourced package. Internally, pnpm identifies this package by its tarball URL (https://codeload.github.com/Khan/mathquill/tar.gz/c9e4329b...) rather than a semver version from a registry. The plain name mathquill (without version) may actually be the most reliable way to match this GitHub-sourced package.

Recommendation

If mathquill@1.0.3 does work as a matcher, it would be worth adding for consistency. If not, a comment explaining why this entry differs from the others would help future maintainers understand the intentional inconsistency.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that we need mathquill listed here. This array lists packages we want to allow pnpm to run install scripts for. Did something break after upgrading pnpm that this fixed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! I should have taken pictures of the failures, but I can revert this to check what those errors were. Is there a record? And what the Claude review said about the tarball thing is what my Claude was saying regarding the version pin.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, perfect! That makes sense.

],
"patchedDependencies": {
"postcss-url": "patches/postcss-url.patch"
Expand Down
Loading
Loading