Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +119 B (+0.02%) Total Size: 486 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (578df27) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3339If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3339If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3339 |
pnpm 10.32.1 no longer hoists @swc/helpers to the top-level node_modules since it's marked optional in the lockfile. This caused Jest to fail resolving @swc/helpers/_/_interop_require_default, which is required when externalHelpers is enabled in .swcrc. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pnpm 10.32.1 blocks git-hosted packages from running build scripts by default unless explicitly listed in onlyBuiltDependencies. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
It's used implicitly via externalHelpers in .swcrc, not imported directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
benchristel
left a comment
benchristel
left a comment
There was a problem hiding this comment.
What's the context for this change? Is this something frontend infra wants us to do?
| "@storybook/test": "^8.6.15", | ||
| "@swc-node/register": "^1.10.10", | ||
| "@swc/core": "1.11.13", | ||
| "@swc/helpers": "^0.5.17", |
There was a problem hiding this comment.
Why do we need @swc/helpers? Is this related to the pnpm update?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
These are the errors this fixed (link to the related check)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward pnpm version bump with mechanical supporting changes.
Extended reasoning...
Overview
This PR bumps pnpm from 10.22.0 to 10.32.1 and makes three small supporting changes: adding @swc/helpers as an explicit devDependency (with a corresponding knip ignore entry), and adding mathquill to pnpm.onlyBuiltDependencies. The lockfile and a changeset file are also updated.
Security risks
No security concerns. The packageManager field includes a SHA-512 hash to pin the exact pnpm binary. The onlyBuiltDependencies change allows mathquill (sourced from Khan's own GitHub repo, pinned to a specific commit in the lockfile) to run build scripts. The @swc/helpers addition is a well-known SWC companion package at a caret-pinned version.
Level of scrutiny
Low scrutiny is appropriate. This is a tooling/infrastructure version bump with no changes to application logic, no changes to production code paths, and no changes to security-sensitive areas. The code changes are mechanical and follow established patterns in the repository.
Other factors
- CI is green (schema check, item splitting, size check, and npm snapshot all passed).
- The two bugs found are both cosmetic nits: a missing trailing newline in the changeset file, and
mathquilllacking a version pin inonlyBuiltDependencies(which may actually be intentional since mathquill is resolved from a GitHub tarball rather than a registry version). - No outstanding reviewer comments to address.
- The +119B bundle size increase is negligible and likely from the
@swc/helpersdependency being resolved slightly differently.
| "esbuild@0.24.0", | ||
| "@swc/core@1.11.13" | ||
| "@swc/core@1.11.13", | ||
| "mathquill" |
There was a problem hiding this comment.
🟡 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.5esbuild@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
- Look at
package.jsonline 147-150: theonlyBuiltDependenciesarray has four entries. - The first three entries all use
name@versionformat. - The fourth entry,
mathquill, has no version qualifier. - In the lockfile, mathquill resolves to version 1.0.3 from
github:Khan/mathquill#v1.0.3(commitc9e4329b0bc5d9b4c21d765b5768e4e7693515b3). - 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah, perfect! That makes sense.
There was a pnpm upgrade in the Dependant removal PR. Tamara and I talked about why it was bumped and I mentioned it would be good to bump it again to latest to take advantage of bug fixes and any security improvements they've made (reviewing the releases since 10.22 there also seems to be some nice quality of life improvements). |
|
Looks like there was also a downgrade of react-router, which affected the WB packaged peer dep resolutions. It could be possible to see runtime errors if any of WB code relied on the v6 APIs (resolved to v5 now) More Review from ClaudeInteresting — the lockfile tells the full story. Here are the three concrete surprise areas:1.
|
Looking at the wonder-blocks packages, it seems like they all have a peer-dependency on |
3 participants
Summary
corepack use pnpm@10Test plan
pnpm installworks correctly in a clean checkoutCo-Authored-By: Claude Opus 4.6 noreply@anthropic.com