refactor(FEC-935): tighten pnpm hoisting + Storybook 9 upgrade#3242
Conversation
🦋 Changeset detectedLatest commit: 178f963 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ByronDWall
left a comment
There was a problem hiding this comment.
Overall a great update, my only question is about workspace versioning the packages that are built within the monorepo
| "lodash": "4.18.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@commercetools-uikit/icons": "20.5.0", |
There was a problem hiding this comment.
can't this be workspace:*?
There was a problem hiding this comment.
It can and will, but I have this as the next step in a separate ticket. This PR is making the implicit dependencies visible, the next one abstracts them.
| "@commercetools-frontend/ui-kit": "20.5.0", | ||
| "@commercetools-local/generator-package-json": "workspace:*", | ||
| "@commercetools-local/generator-readme": "workspace:*", | ||
| "@commercetools-uikit/design-system": "20.5.0", | ||
| "@commercetools-uikit/hooks": "20.5.0", | ||
| "@commercetools-uikit/i18n": "20.5.0", | ||
| "@commercetools-uikit/utils": "20.5.0", |
There was a problem hiding this comment.
can @commercetools-frontend/ui-kit and @commercetools-uikit/* packages be resolved as workspace packages so that we don't have to update the versions after each release?
Drop shamefully-hoist=true in favour of declared-deps-only. Adds an ESLint override for docs snippets (per-package decision: snippets show consumer perspective, not the documenting workspace's deps), hoists @storybook/* via public-hoist-pattern, and declares the previously-phantom transitives across every affected workspace (lodash, react-router-dom, react-select, history, prop-types, @emotion/styled, popper.js, react-is, jsdom, shelljs, @storybook/addon-docs, plus workspace-internal deps). Two follow-up blockers — see PR description: - storybook build picks up @storybook/react's bundled template stories (Button.jsx/Page.jsx) under strict pnpm; structural Storybook 8 issue, fixed in Storybook 9. - ~40-50 typecheck residuals not yet audited. Acceptance criteria from FEC-935 not yet met; PR opens as draft.
Upgrades the storybook workspace to the latest 9.x line via
`pnpm dlx storybook@9.1.20 upgrade`, plus targeted manual cleanups.
Bundles the SB9 migration into FEC-935 so the branch lands the way
the strict-pnpm posture should look long-term.
Why bundled with FEC-935:
Under strict pnpm the storybook build was tripping on Storybook 8's
`@storybook/react/template/cli/js/{Button,Page}.jsx` template files,
which the workspace stories glob accidentally matched. Storybook 9's
flatter dependency graph removes the offending `template/cli/`
directory entirely, so the FEC-935 .npmrc no longer needs to lean on
a broad `@storybook/*` carve-out for that specific reason.
What changed:
- `storybook/package.json`: drop packages consolidated into core
(`@storybook/addon-essentials`, `addon-interactions`,
`addon-storysource`, `blocks`, `manager-api`, `react`, `test`,
`theming`) and drop unused ones (`addon-themes`, `addon-onboarding`).
Remaining `@storybook/*` deps and core `storybook` pinned at 9.1.20.
- 92 `*.stories.tsx`: `from '@storybook/react'` → `'@storybook/react-vite'`.
- 80 `*.readme.mdx`: `from '@storybook/blocks'` → `'@storybook/addon-docs/blocks'`.
- `storybook/.storybook/manager.ts`: `@storybook/manager-api` → `storybook/manager-api`.
- `storybook/.storybook/theme.ts`: `@storybook/theming/create` → `storybook/theming/create`.
- `storybook/src/decorators/*-decorator.tsx`: `Decorator` type → `@storybook/react-vite`.
- `storybook/.storybook/main.ts`: addons array trimmed (essentials/interactions/
storysource merged into core), framework name wrapped in `getAbsolutePath`.
- Stories glob anchored to a literal `src/` segment so it can't descend into
the nested `<pkg>/node_modules/@commercetools-uikit/*` symlinks pnpm leaves
for workspace deps (was tripping the indexer's duplicate-id check).
.npmrc:
The branch's broad `public-hoist-pattern[]=@storybook/*` carve-out stays —
the namespace hoist is justified independent of the v8 template bug:
leaf packages import Storybook-namespaced types from a framework-user
perspective (matching the ESLint `**/docs/**` override), and declaring
~15 `@storybook/*` packages on 80+ leaf workspaces would be pure noise.
The carve-out is still ~1000x narrower than `shamefully-hoist=true`.
Validation:
- pnpm install: clean under strict pnpm (29 added, 55 removed — SB9's
"less than half the size of SB8" promise paying off)
- pnpm build: green
- pnpm test: 1402 passing, 0 failing
- pnpm lint: 1308 passing, 0 failing (after dropping an orphan
`eslint-plugin-storybook` import the automigration left at root)
- pnpm lint:publint: green (warnings only)
- pnpm --filter storybook build: green (was the original FEC-935 blocker)
- pnpm typecheck: 273 errors — exact match with d5e4725 baseline;
SB9 upgrade introduces zero new typecheck errors. Residuals are
pre-existing tech debt (114 implicit-any in CSF2 StoryFn callbacks
+ emotion styled template literals, 70 unused @ts-expect-error
directives, 89 type mismatches in data-table-manager and similar).
Two issues surfaced once shamefully-hoist=true was dropped: - tsconfig.json: flip preserveSymlinks to false. Under strict pnpm, every dep is a symlink (node_modules/.pnpm/<pkg>/node_modules/<pkg>/). preserveSymlinks=true makes TypeScript resolve type imports from the symlink location instead of the real path, breaking peer-dep subpath imports like @storybook/react -> 'storybook/internal/types'. AnnotatedStoryFn/StoryFn/Meta/Decorator degraded to any-ish shapes, cascading to 272 errors across stories and data-table-manager (TS7006 implicit-any in StoryFn callbacks, TS2578 newly-unused @ts-expect-error suppressions, TS2339/TS2559 type mismatches). The flag predated pnpm and was a no-op under yarn classic flat hoisting. - packages/components/tooltip: declare @types/react-is (transitively visible under shamefully-hoist, invisible under strict pnpm). Also bump accessible-button's @types/react-is to 19.2.0 for manypkg compliance. pnpm typecheck: 273 -> 0 errors.
visual-testing-app's vite build surfaced more strict-pnpm phantom-dep gaps in visualroute fixtures (was previously masked by typecheck failure). Same FEC-935 pattern as the rest of the migration: shamefully-hoist flattened everything to root, strict pnpm requires each workspace to declare what it imports. **Self-references in visualroute files (Option A: relative imports)** Four visualroute fixtures imported their own workspace by package name (e.g. `from '@commercetools-uikit/pagination'` inside pagination/src/). Under strict pnpm, pnpm doesn't create a self-symlink into a workspace's own node_modules, so the name was unresolvable. Rewritten to relative imports — matches the established pattern in this repo (story files, decorators, etc. all use relative imports for in-workspace files). Concerns: introducing a self-devDep is an unusual pattern not used anywhere else in the codebase and pnpm explicitly warns about cyclic deps in their workspaces guide. Files: pagination, card, grid, icons (4 imports — main + 3 subpath entries), design-system. **Cross-workspace imports (Option B: declare devDeps)** Visualroute fixtures importing other workspaces by package name (consumer's-perspective style) now declare those workspaces as devDependencies — matches FEC-935's broader "declare what you use" philosophy. - card: +@commercetools-uikit/text - icons: +@commercetools-uikit/text, +@commercetools-uikit/spacings - stamp: +@commercetools-uikit/icons - grid: +@commercetools-uikit/design-system - text: +@emotion/styled - progress-bar: +@emotion/styled **Router phantom deps (Option B)** Seven visualroute fixtures use react-router(-dom) for declarative URL routing within the Percy test app. All seven workspaces now declare it as a devDep pinned to 5.3.4 (matching visual-testing-app). - rich-text-input: +react-router - design-system: +react-router - icons, primary-action-dropdown, async-creatable-select-field, async-select-field, select-input, localized-rich-text-input: +react-router-dom **Root devDep additions** Two test/percy helpers (suite.jsx, spec.jsx, local-theme-provider.jsx) and one orphan visualroute fixture (packages/components/spacings/ spacings.visualroute.jsx — sits at a path with no owning workspace, since `spacings/` has 4 sub-workspaces but no parent package.json) import packages that need to be resolvable from the repo root. - @commercetools-uikit/design-system, hooks, i18n, utils - @emotion/styled, prop-types, lodash pnpm install / build / test / lint / lint:publint / storybook build / visual-testing-app build / typecheck — all green.
The four FEC-935 commits were rebased onto post-hotfix main (PR #3243), which added `link-workspace-packages=true` + `prefer-workspace-packages=true` to .npmrc. The rebase took --theirs for the lockfile during the first conflict; this regeneration reconciles the lockfile with the merged .npmrc state (strict pnpm hoisting + workspace linking). Net: ~700 internal cross-deps now resolve through workspace symlinks (`version: link:`) instead of registry tarballs, matching what the hotfix established on main. Note: the workspace-linking .npmrc settings remain as a short-term unblock. The canonical fix is to convert internal `@commercetools-uikit/*` cross-deps from concrete `"20.5.0"` specifiers to `"workspace:^"`, deferred to FEC-938 (post-pnpm tooling polish) since it fits naturally alongside pnpm catalogs adoption and is unrelated to FEC-935's hoisting scope.
dd3d75f to
178f963
Compare
Summary
FEC-935 drops
shamefully-hoist=truefrom.npmrcin favor of strict pnpm with a single narrow carve-out (public-hoist-pattern[]=@storybook/*), and bundles the Storybook 8.6.18 → 9.1.20 upgrade that structurally resolves the v8 template-stories pickup bug.What changed
Drop shamefully-hoist (the FEC-935 acceptance criterion)
.npmrc:shamefully-hoist=trueremoved; one targetedpublic-hoist-pattern[]=@storybook/*carve-out documented inline.**/docs/**&**/_docs/**so consumer-perspective docs snippets don't require leaf packages to declare framework-level deps.@types/*for transitives previously resolved by hoisting.CONTRIBUTING.mdnote aboutgit worktree+ nestednode_modulescausing "two copies of React" when the worktree sits inside an installed parent repo.Bundled: Storybook 8.6.18 → 9.1.20
Rolled in so the strict-pnpm posture lands the way it should look long-term. Storybook 9's flatter dep graph removes the v8-era
@storybook/react/template/cli/js/{Button,Page}.jsxfiles that were tripping the stories glob under strict pnpm.storybook/package.json: 8 consolidated packages dropped (essentials, interactions, storysource, blocks, manager-api, react, test, theming → all merged into core in v9); remaining@storybook/*andstorybookpinned at 9.1.20.*.stories.tsx: type imports@storybook/react→@storybook/react-vite.*.readme.mdx: doc-block imports@storybook/blocks→@storybook/addon-docs/blocks.manager.ts,theme.ts, decorators,preview.tsx: updated to v9 import paths.main.tsanchored to a literalsrc/segment so it cannot descend into the nested<pkg>/node_modules/@commercetools-uikit/*symlinks that strict pnpm leaves for workspace deps.Typecheck regressions surfaced by strict pnpm
Two latent issues — invisible under shamefully-hoist — were exposed when strict pnpm replaced the flat layout with per-package symlinks. Together they caused 273 typecheck errors; both fixed surgically.
tsconfig.json:preserveSymlinks: true→false. Predated pnpm; harmless under yarn classic flat hoisting. Under strict pnpm every dep is a symlink (node_modules/.pnpm/<pkg>/node_modules/<pkg>/), andpreserveSymlinks: truemade TypeScript resolve type imports from the symlink location instead of the real path. That broke peer-dep subpath imports —@storybook/react'simport { … } from 'storybook/internal/types'returned an unresolved shape, degradingAnnotatedStoryFn/StoryFn/Meta/Decoratorto any-ish. Cascade: 114 TS7006 implicit-any inStoryFn<Props>callbacks, 70 TS2578 newly-unused@ts-expect-errorsuppressions, 89 TS2339/TS2559/TS2769/TS2322/TS7031/TS2741 type mismatches indata-table-managerand emotion-styled props.packages/components/tooltip: declare@types/react-is: 19.2.0. Tooltip imports fromreact-isbut never declared the types package; transitively visible under shamefully-hoist, invisible under strict pnpm. Also bumpedaccessible-button's@types/react-isfrom^19.0.0→19.2.0for manypkg compliance.Visualroute phantom-dep regressions (visual-testing-app build)
Same FEC-935 pattern, surfaced once typecheck stopped masking later CI stages. Visualroute fixtures (co-located in component workspaces, consumed by visual-testing-app's vite build for Percy snapshots) imported packages that weren't declared on the owning workspace. Split into two categories with explicit rationale:
Self-references → relative imports (Option A). Four visualroute fixtures imported their own workspace by package name (e.g.
from '@commercetools-uikit/pagination'insidepagination/src/). Under strict pnpm there's no self-symlink, so the name was unresolvable.Rewrote to relative imports (
from '..'). Rationale:"@me": "workspace:*") was experimentally validated to work technically, but pnpm warns about cyclic deps generally and the pattern doesn't exist anywhere else in the codebase. Wouldn't introduce a new norm for a single edge case."exports"field for self-reference) is out of scope here — would change consumer resolution semantics.Files:
pagination,card,grid,icons(main + 3 subpath entries),design-system.Cross-workspace imports → declare devDeps (Option B). Visualroute fixtures importing other workspaces by package name (consumer's-perspective style) now declare those workspaces as devDependencies — matches FEC-935's broader "declare what you use" philosophy and industry consensus across pnpm/Nx/Turbo docs.
card +text,icons +text +spacings,stamp +icons,grid +design-system,text +@emotion/styled,progress-bar +@emotion/styled.react-routerorreact-router-dompinned to5.3.4(matchingvisual-testing-app).test/percy/*.jsx) and one orphan visualroute fixture (packages/components/spacings/spacings.visualroute.jsx— parent dir has 4 sub-workspaces but no parentpackage.json, so the fixture's nearest workspace is the repo root):@commercetools-uikit/design-system/hooks/i18n/utils,@emotion/styled,prop-types,lodash.Validation matrix
pnpm install(strict pnpm)pnpm buildpnpm testpnpm lintpnpm lint:publintpnpm --filter storybook buildpnpm --filter visual-testing-app buildpnpm typecheckDecisions captured
public-hoist-pattern[]=@storybook/*retained — ~15 packages hoisted, all conceptually part of the storybook toolchain. Leaf packages import Storybook-namespaced types from a framework-user perspective (matching the ESLint**/docs/**override). Dramatically narrower thanshamefully-hoist=true(~1000 packages).pnpm dlx storybook@9.1.20 upgradehandled the bulk of the codemod; manual pass cleaned up the orphaneslint-plugin-storybookimport the automigration left at root.Refs