[PF-1993] Migrate from Yarn 1 to pnpm 10 - #4920
Merged
Merged
Conversation
- Delete yarn.lock; pnpm-lock.yaml is now derived from yarn.lock via pnpm import (resolved versions preserved across all root and transitive deps). - Add 90 workspace packages as devDependencies of root so .storybook/* and tailwind.config.js can resolve them under pnpm (yarn 1 auto- hoisted via the workspaces field; pnpm requires explicit declaration). Refresh tsconfig references for the resulting workspace graph. - Restore @storybook/core-server>ip alias to @toptal/davinci-ip@2.0.3 to mitigate GHSA-2p57-rm9w-gvfp (the override was lost in the resolutions -> pnpm.overrides translation). - Drop js-yaml ^3.13.1 override that was forcing every consumer to v3 and downgrading lerna, semantic-release, jsdom, and others that yarn.lock had on v4. - Replace './node_modules/...' hardcoded ESLint extends path and the ../node_modules/.bin/tsc path in bin/build.js with require.resolve so resolution doesn't depend on the linker mode. - Switch the alpha publish action from npx to pnpm exec for deterministic resolution against the locally installed binary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The pnpm config (node-linker, auto-install-peers, link-workspace-packages,
resolve-peers-from-workspace-root) was on disk but gitignored, so CI
checked out without it and silently fell back to pnpm's default
node-linker=isolated. That broke type resolution for @types/react,
@types/node, and @types/lodash from picasso-provider/picasso-shared
because optional-peer types weren't being hoisted to root node_modules.
The token placeholder uses ${NPM_TOKEN}, no secret is baked in.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Since .npmrc is now committed to the repo, the two COPY steps already place the correct file in /app. The printf was overwriting it with an auth-only version that dropped node-linker=hoisted and the rest of the pnpm config — any pnpm install inside the runtime image would have silently switched to the isolated linker. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
denieler
changed the title
GitHub's dependency-graph compare API returns 502 when diffing yarn.lock <-> pnpm-lock.yaml across the migration boundary. The action just relays the upstream error (tracked in actions/dependency-review-action#398). The check resumes working for every PR once master holds pnpm-lock.yaml. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The picasso storybook bundle hits the default ~4 GiB heap on ubuntu-latest and aborts with FATAL ERROR: Allocation failed - JavaScript heap out of memory. The Storybook 6 webpack5 builder loads every package's stories into a single bundle and exceeds the default cap. Apply NODE_OPTIONS via the existing cross-env wrapper so it applies to both build:storybook (used by ci.yaml deploy + release.yml) and happo:storybook (visual-testing.yml + ci.yaml happo job). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Post Cache built packages step was hanging indefinitely. Under pnpm with link-workspace-packages=true, every workspace package gets internal symlinks like packages/<a>/node_modules/@toptal/<b> -> packages/<b>. actions/cache@v4 uses @actions/glob which follows symlinks for **, so packages/**/dist-package enumerated forever through the symlink loops. Replace with explicit non-** patterns that match the two real workspace tiers. Also drop the stray }} that was on the cache key. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pnpm's link-workspace-packages=true creates symlinks like packages/picasso/node_modules/@toptal/picasso-provider -> packages/picasso-provider, which itself contains a symlink back to packages/picasso. Webpack's _resolveContextTimestamp walks each entry's symlinks recursively without dedup, so the cycle blows the local hashes array past 2^32-1 and Array.push throws "RangeError: Invalid array length" at FileSystemInfo.js:3593. The fix is to mark the relevant trees as managedPaths so _readContext short-circuits via fromManagedItem before reaching fromSymlink. Webpack matches managedPaths against the path being read; the symlink targets are workspace package directories (packages/<x>), not node_modules paths, so the node_modules regex alone wasn't enough. Add the workspace package tiers as plain string entries (more specific first so packages/base/<Comp> wins over packages/<x>). Verified locally: pnpm test:integration runs all 57 specs, 289/291 passing (2 skipped, no failures). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GitHub-hosted ubuntu runners have 16 GB physical RAM. The previous 8 GB cap was hitting FATAL ERROR: Reached heap limit on the preview bundle for picasso's 90-package design system. 12 GB leaves ~4 GB for thread-loader workers and the runner's own overhead. Doesn't reduce wall-time (compile speed is unchanged); just stops the OOM. If wall-time is still painful, next step is bigger runners. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Source-map generation on the 90-package preview bundle costs ~2-3 GB peak heap and 20-30% wall time. Output goes to gh-pages and happo where maps aren't useful — strip them on production builds, keep cheap-module-source-map for `pnpm start:storybook`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without snapshot.managedPaths, webpack's _resolveContextTimestamp
recurses through pnpm's circular workspace symlinks
(packages/<x>/node_modules/@toptal/<y> -> packages/<y>) and the
storybook build silently hangs after "Compiling preview..", with no
output until the runner times out. Cypress had the same issue (fixed in
cypress.config.mjs). Apply the same managedPaths shape here:
- regex covering any */node_modules/* path
- workspace package directories themselves (where the symlinks point)
so _readContext short-circuits via fromManagedItem before reaching
fromSymlink. More specific path first so packages/base/<Comp>
matches before packages/<x>.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the fix used in staff-portal's pnpm migration
(toptal/staff-portal patches/webpack+5.95.0.patch). Webpack 5.98.0's
FileSystemInfo._resolveContextTimestamp /
_resolveContextHash / _getContextTimestampAndHash recurse through
entry.symlinks without dedup. Under pnpm's hoisted linker that creates
circular workspace symlinks
(packages/<x>/node_modules/@toptal/<y> -> packages/<y>), the recursion
either hangs the build for an hour ("Compiling preview..") or grows the
hashes array past 2^32-1 and throws RangeError: Invalid array length.
Patch adds a processedSymlinks Set at each of the three call sites and
skips already-visited targets, breaking the cycle.
Picasso pins webpack to 5.98.0 via pnpm.overrides; the staff-portal
patch context lines line up exactly at 3598/3713/3890.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
denieler
commented
May 5, 2026
denieler
commented
May 5, 2026
denieler
commented
May 5, 2026
The migration helper bin/add-missing-workspace-deps.mjs treated *.example.tsx as a "test/story" file and added every import in those files to the package's devDependencies. But the *.example.tsx files are storybook code-sample templates and the __testfixtures__/ files are codemod fixtures — neither is real consumer code, so the imports shouldn't drive package metadata. This commit prunes 111 such bogus devDependencies (workspace packages only) across 76 packages. Build still passes for all 90 projects. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
denieler
commented
May 5, 2026
denieler
commented
May 5, 2026
The migration helper bin/add-missing-workspace-deps.mjs added a number of cross-workspace deps to individual package.json files based on imports it found in source. Many of those imports were in *.example.tsx (storybook code-sample templates) or __testfixtures__/ (codemod input/output fixtures) — neither of which is real consumer code, so they shouldn't drive the package's published metadata. Rather than try to filter the right deps out, restore the dependencies / devDependencies / peerDependencies blocks of every workspace package to the master version. Other fields (scripts, version, etc.) keep the migration changes. Refresh tsconfig project references for the new (smaller) workspace dep graph. pnpm install + build:package both still succeed for all 90 projects. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The rollback commit (60f65eb) updated package.json files but I forgot to stage the regenerated pnpm-lock.yaml; CI's frozen install saw the mismatch (manifest "*" vs lockfile "5.0.2" for @toptal/picasso-provider in packages/picasso) and aborted with ERR_PNPM_OUTDATED_LOCKFILE. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
denieler
commented
May 6, 2026
| @@ -1,8 +1,22 @@ | |||
| import React from 'react' | |||
| import { render, waitFor } from '@toptal/picasso-test-utils' | |||
| import { render as baseRender, waitFor } from '@testing-library/react' | |||
Collaborator
Author
There was a problem hiding this comment.
to remove circularity test-utils -> provider -> shared -> test-utils
Contributor
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
Contributor
|
🎉 Alpha packages are ready! PR: #4920 Installation commands: These alpha packages were built from the latest commit in this PR. |
vedrani
approved these changes
May 7, 2026
vedrani
left a comment
vedrani
left a comment
Collaborator
There was a problem hiding this comment.
Looks great 💯
Just a couple of things to check. Claude is throwing couple of concerns about docker prune and caching.
| # syntax=docker/dockerfile:1 | ||
| FROM node:18-alpine | ||
| ARG NODE_VERSION=22.20.0 | ||
| ARG PNPM_VERSION=10.32.1 |
Collaborator
There was a problem hiding this comment.
We can move this to package.json packageManager to have one single point of truth for version
https://github.com/toptal/staff-portal/blob/master/package.json#L153
| COPY --chown=node:node packages/base/Utils/package.json ./packages/base/Utils/package.json | ||
| # Prune to manifests so dependency installation stays cache-friendly. | ||
| RUN find packages -mindepth 2 -maxdepth 2 \! -name "package.json" -print | xargs rm -rf | ||
| RUN find packages/base -mindepth 2 -maxdepth 2 \! -name "package.json" -print | xargs rm -rf |
Collaborator
There was a problem hiding this comment.
After the first RUN, packages/base/ is empty, so the subsequent pnpm install --frozen-lockfile cannot resolve any of the 70+ base workspace packages and the image build will fail.
Or it's fine because you copied them?
| # Enables layer caching: copy lockfile + workspace manifests first, | ||
| # install, then copy the rest of the source. | ||
| COPY --chown=node:node package.json pnpm-lock.yaml pnpm-workspace.yaml .npmrc lerna.json ./ | ||
| COPY --chown=node:node packages packages |
Collaborator
There was a problem hiding this comment.
The new flow is COPY packages packages → RUN find … rm -rf → RUN pnpm install. The first COPY layer's hash depends on every file under packages/, so the install layer invalidates on every code change.
| "micromatch": "^4.0.8", | ||
| "nx": "21.5.1", | ||
| "@nx/js": "21.5.1", | ||
| "@types/react": "17" |
Collaborator
Author
There was a problem hiding this comment.
@vedrani we still use react 17 in some nested packages, so I used the "minimal compatible version"
Collaborator
Author
There was a problem hiding this comment.
@vedrani it seems this is the blocker to migrate to types from v18:
The build fails because packages/base/Utils/src/utils/forward-ref.ts:33-38 uses the older React 17 forwardRef signature. React 18 types tighten forwardRef's argument to ForwardRefRenderFunction<T, PropsWithoutRef<P>>. This is a real reason to keep 17 — unless we fix the helper. Let me try a targeted cast.
looks legit to me, but probably easy to fix also 🤔
dulishkovych
approved these changes
May 7, 2026
Contributor
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
Contributor
|
🗑️ Storybook preview cleaned up The preview deployment has been automatically removed since this PR was closed. |
3 tasks
denieler
added a commit
that referenced
this pull request
May 7, 2026
dulishkovych
added a commit
that referenced
this pull request
May 14, 2026
`nx release publish --tag=canary` has been failing on every PR
dispatched against the post-pnpm-migration master because Nx
detects a four-edge circular dependency in the task graph:
picasso-provider ─→ picasso-notification
│
▼
picasso-test-utils ←── picasso-tailwind-merge
│
└─ (prod-dep) ─→ picasso-provider (closes the loop)
This cycle has existed structurally in `package.json` declarations
for a long time. Before PR #4920 ("PF-1993 Migrate from Yarn 1 to
pnpm 10") it was suppressed by an `nx.implicitDependencies`
negation hint inside `picasso-provider/package.json`:
"nx": { "implicitDependencies": ["!@toptal/picasso-test-utils"] }
PR #4920 also removed picasso-provider's explicit devDep on
picasso-test-utils, and treated the nx hint as dead code from that
removal — but the hint was load-bearing for the larger cycle that
runs *through* notification and tailwind-merge, not just provider's
own outgoing edge. Removing the hint exposed the four-edge cycle to
Nx for the first time since the migration.
This commit reinstates an equivalent hint at the safest edge of the
cycle: the devDependency from picasso-tailwind-merge to
picasso-test-utils. Picking this edge has minimal risk because:
* It's a devDep, not a production dep — real consumers of
tailwind-merge never receive test-utils transitively.
* tailwind-merge's own tests still get test-utils via `node_modules`
at install time; only Nx's task-graph view is affected.
* The three remaining edges (all production deps) still encode the
correct build/publish order: provider → notification →
tailwind-merge.
No source code change, no public API change, no lockfile change.
Local `pnpm install`, `pnpm tsc:all`, `pnpm lint`, `pnpm test:unit`
all behave identically before and after — those don't consult Nx's
project graph.
Follow-up for a separate ticket: convert picasso-test-utils' regular
dep on picasso-provider to a peer dep, which would break the cycle
structurally rather than via an Nx hint.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dulishkovych
added a commit
that referenced
this pull request
May 14, 2026
`nx release publish --tag=canary` has been failing on every PR
dispatched against the post-pnpm-migration master because Nx
detects a four-edge circular dependency in the task graph:
picasso-provider ─→ picasso-notification
│
▼
picasso-test-utils ←── picasso-tailwind-merge
│
└─ (prod-dep) ─→ picasso-provider (closes the loop)
This cycle has existed structurally in `package.json` declarations
for a long time. Before PR #4920 ("PF-1993 Migrate from Yarn 1 to
pnpm 10") it was suppressed by an `nx.implicitDependencies`
negation hint inside `picasso-provider/package.json`:
"nx": { "implicitDependencies": ["!@toptal/picasso-test-utils"] }
PR #4920 also removed picasso-provider's explicit devDep on
picasso-test-utils, and treated the nx hint as dead code from that
removal — but the hint was load-bearing for the larger cycle that
runs *through* notification and tailwind-merge, not just provider's
own outgoing edge. Removing the hint exposed the four-edge cycle to
Nx for the first time since the migration.
This commit reinstates an equivalent hint at the safest edge of the
cycle: the devDependency from picasso-tailwind-merge to
picasso-test-utils. Picking this edge has minimal risk because:
* It's a devDep, not a production dep — real consumers of
tailwind-merge never receive test-utils transitively.
* tailwind-merge's own tests still get test-utils via `node_modules`
at install time; only Nx's task-graph view is affected.
* The three remaining edges (all production deps) still encode the
correct build/publish order: provider → notification →
tailwind-merge.
No source code change, no public API change, no lockfile change.
Local `pnpm install`, `pnpm tsc:all`, `pnpm lint`, `pnpm test:unit`
all behave identically before and after — those don't consult Nx's
project graph.
Follow-up for a separate ticket: convert picasso-test-utils' regular
dep on picasso-provider to a peer dep, which would break the cycle
structurally rather than via an Nx hint.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates picasso's package manager from Yarn 1 to pnpm 10. This unblocks
@toptal/davinci-syntax@24(which requires pnpm) and aligns picasso with the rest of Toptal's frontend tooling.pnpm importto preserve every resolved version fromyarn.lock(verified by byte-identical reproduction and per-package sampling)..storybook/*andtailwind.config.jsresolve them — yarn 1 auto-hoisted via theworkspacesfield, pnpm doesn't.tsconfigproject references regenerated for the new workspace graph.pnpm-installcomposite action handles corepack + pnpm-store cache +--frozen-lockfile).Dockerfileswitched tocorepack+pnpm install --frozen-lockfile.Override changes vs old
resolutions@storybook/core-server>ip→@toptal/davinci-ip@2.0.3(mitigates GHSA-2p57-rm9w-gvfp; the alias had been dropped in the WIP).js-yaml ^3.13.1global override — it was forcing every consumer to v3 and downgradinglerna,semantic-release,jsdom,cosmiconfig, etc. that yarn.lock had on v4.Other fixes
.eslintrc.jsextends path →require.resolve(...)(no longer depends on linker mode).bin/build.jstsc path →require.resolve('typescript/bin/tsc').npx davinci-engine→pnpm exec davinci-engine(deterministic resolution against the just-installed binary).Storybook
Intentionally not upgraded — staying on Storybook 6 + the existing
@storybook/react ^6.5.15override.Test plan
Local validation passed:
pnpm install --frozen-lockfilepnpm lint(0 errors)pnpm typecheckpnpm build:package(90 projects)pnpm test:unit:cipnpm build:storybookresolution clean (manager bundle built; preview bundle OOM'd locally — bundle size; CI runners have sufficient memory)Watch in CI:
davinci-integration-tests.yml— Cypress canary against the built artifactsvisual-testing.yml— Happo storybook smoke