Skip to content

chore(deps): make node_modules strict, declare what each package imports - #5327

Open
Han5991 wants to merge 1 commit into
facebook:mainfrom
Han5991:chore/drop-node-linker-hoisted
Open

chore(deps): make node_modules strict, declare what each package imports#5327
Han5991 wants to merge 1 commit into
facebook:mainfrom
Han5991:chore/drop-node-linker-hoisted

Conversation

@Han5991

@Han5991 Han5991 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Closes #5328

What

Removes both hoisting settings from pnpm-workspace.yaml and declares the dependencies that were only resolving because of them. linkWorkspacePackages: true stays.

Why

Both lines came over verbatim from the pre-pnpm-11 .npmrc (#3700).

nodeLinker: hoisted was annotated there as "Use the Node-modules linker (default), not pnpm's PnP." That is wrong on its own terms — pnpm's default is the node-modules linker, but its default mode is isolated, not hoisted. It also paired badly: nodeLinker: hoisted together with shamefullyHoist: true is pnpm/pnpm#6620, spurious ModuleNotFoundErrors that only clear after toggling the setting off and on.

shamefullyHoist: true is publicHoistPattern: '*' — it republishes every transitive package at the root of node_modules, so a package can import a module it never declared and nothing complains.

Benefits

  • Undeclared imports become errors, permanently. Eight of them were live in this repo (below). After this, CI catches the next one instead of it riding on a hoisted copy.
  • Dedup is restored. isolated hardlinks each package once into the .pnpm store instead of writing a flat npm-style tree.
  • Version conflicts resolve correctly. Each importer gets its own exact version, instead of one winner hoisted to the workspace root when two projects disagree.

What was hiding behind the flag

Turning it off surfaced exactly eight undeclared imports:

Package Undeclared Symptom
root scripts @babel/core, glob, chokidar, @testing-library/jest-dom, @stylexjs/babel-plugin pnpm build fails at @astryxdesign/core
lab, charts, richtext @babel/cli, @babel/core, @babel/preset-react, @babel/preset-typescript, @stylexjs/babel-plugin build:esmbabel: command not found
lab, charts, richtext @testing-library/react 27 test files load 0 tests
core @types/babel__core TS7016
storybook @heroicons/react, @stylexjs/stylex, @lexical/react, lexical build fails to resolve
docsite @babel/parser type error after a clean compile
cli @heroicons/react, @stylexjs/stylex, lucide-react, recharts assets/templates/** is in tsconfig.strict.json, which CI runs

All are devDependencies — nothing changes for consumers of the published packages. Every version matches what the workspace already resolved, so the lockfile gains importer entries and zero new package versions.

Risks, and how each was closed

Risk Resolution
Build tools that walk node_modules break — the original stated reason for the flag, naming Storybook and the StyleX babel plugin pnpm build, storybook:build, docsite build and sandbox build all pass
A CI typecheck gate breaks Every typecheck CI runs passes, including cli typecheck:strict — the one that compiles the 145-error template assets
Tests regress 541/548 files pass with 0 module-resolution errors — identical to the same run with hoisting on
Symlink-hostile environment (Docker, serverless, React Native) None in this repo; all CI runners are Ubuntu, no Dockerfile, no react-native
Workspace packages silently resolving from the registry linkWorkspacePackages: true untouched — required, since 64 internal deps are bare "*" (only 4 use workspace:*) and pnpm ≥ 10 defaults it to false

Pre-existing, left untouched

  • 7 failing test files (plainDate, Calendar, DateInputTouch, Schedule, TransferListSelector, 2 CLI suites). Baselined against main with hoisting restored: the same 7 fail there, with more failing cases (69 vs 56).
  • lab typecheck — relative .doc.mjs imports plus a Drawer onClose prop error. Unrelated to module resolution; CI runs lab typecheck:docs, which passes.
  • template-viewer build — fails identically with hoisting restored. packages/build aliases the @astryxdesign/core prefix to core/src, so core/locales/*.json rewrites to a path that does not exist. CI does not build this app.

Note for reviewers pulling this branch

Changing hoisting makes pnpm recreate node_modules wholesale. Interactively it prompts; in a non-TTY shell it aborts with ERR_PNPM_ABORTED_REMOVE_MODULES_DIR_NO_TTY unless CI=true is set. GitHub Actions sets CI itself, so CI is unaffected.

@vercel

vercel Bot commented Aug 22, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
astryx Ready Ready Preview Aug 22, 2026 8:53am

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 22, 2026
@github-actions github-actions Bot added community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge labels Aug 22, 2026
@github-actions

github-actions Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

No new or modified components detected.

Bundle Size Summary

No component packages changed.

Accessibility Audit

Status: No accessibility violations detected.


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

github-actions Bot added a commit that referenced this pull request Aug 22, 2026
Drops `nodeLinker: hoisted` and `shamefullyHoist: true`, both inherited
verbatim from the pre-pnpm-11 .npmrc (facebook#3700). nodeLinker was annotated there
as pnpm's default, which it is not -- the default is `isolated` -- and pairing
it with shamefullyHoist is pnpm/pnpm#6620. shamefullyHoist is
`publicHoistPattern: '*'`, which let nine packages import modules they never
declared; all nine are now declared, at the versions the workspace already
resolved. The Babel toolchain and Testing Library move into the catalog, each
having been duplicated across 4-6 packages.

Also fixes a latent `astryx doctor` bug the linker change exposed:
checkPeerDeps resolved peers with `require.resolve(name, {paths: [cwd]})`, but
Node folds NODE_PATH in regardless, so a peer merely reachable from the ambient
environment read as installed and doctor stayed silent about a missing one. It
now walks the project's own node_modules and reads package.json off disk, so
every peer gets range-checked. The existing test caught this unmodified.

Verified on a recreated node_modules: build, test, storybook:build, docsite and
sandbox builds, check:repo, verify-exports, eslint, and every typecheck CI gates
all pass. The local test failures that remain reproduce on main from a clean
worktree -- case-insensitive filesystem, date-dependent suites -- and CI
reported none of them.
@Han5991
Han5991 force-pushed the chore/drop-node-linker-hoisted branch from 6808c5a to 17d3051 Compare August 22, 2026 08:50
github-actions Bot added a commit that referenced this pull request Aug 22, 2026
@Han5991
Han5991 marked this pull request as ready for review August 22, 2026 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Infra] shamefullyHoist hides undeclared imports; nodeLinker: hoisted is redundant

1 participant