chore: forbid react-icons, griffel and react-motion in v9 base hooks and headless - #36544
chore: forbid react-icons, griffel and react-motion in v9 base hooks and headless#36544Martin Hochel (Hotell) wants to merge 7 commits into
Conversation
📊 Bundle size report✅ No changes found |
|
Pull request demo site: URL |
There was a problem hiding this comment.
Pull request overview
Aligns v9 base-hook linting with bundle-isolation restrictions and records existing dependency debt.
Changes:
- Adds workspace-aware package ownership and structural type analysis.
- Expands forbidden runtimes and bundle checks.
- Adds fixtures, tests, suppressions, and change records.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tools/eslint-rules/rules/base-hook-no-forbidden-runtime.ts |
Implements structural type and workspace ownership analysis. |
tools/eslint-rules/rules/base-hook-no-forbidden-runtime.spec.ts |
Tests workspace and omitted-type behavior. |
tools/eslint-rules/rules/__fixtures__/base-hook-no-forbidden-runtime/tsconfig.json |
Maps new fixture packages. |
tools/eslint-rules/rules/__fixtures__/base-hook-no-forbidden-runtime/stubs/workspace-runtime/package.json |
Defines the workspace runtime fixture. |
tools/eslint-rules/rules/__fixtures__/base-hook-no-forbidden-runtime/stubs/workspace-runtime/index.ts |
Provides fixture runtime exports. |
tools/eslint-rules/rules/__fixtures__/base-hook-no-forbidden-runtime/stubs/workspace-relay-pkg/index.ts |
Re-exports workspace runtime symbols. |
tools/eslint-rules/rules/__fixtures__/base-hook-no-forbidden-runtime/stubs/watched-pkg/index.ts |
Exports structural type fixtures. |
tools/eslint-rules/rules/__fixtures__/base-hook-no-forbidden-runtime/stubs/watched-pkg/heavy.ts |
Adds styled and omitted props fixtures. |
tools/eslint-rules/rules/__fixtures__/base-hook-no-forbidden-runtime/stubs/heavy-runtime/package.json |
Adds fixture package identity. |
tools/eslint-rules/rules/__fixtures__/base-hook-no-forbidden-runtime/stubs/cyclic-heavy-pkg/package.json |
Adds cyclic fixture package identity. |
packages/react-components/react-message-bar/library/src/components/MessageBar/useMessageBar.ts |
Records existing motion coupling. |
packages/react-components/react-menu/library/src/components/Menu/useMenu.tsx |
Records existing Griffel coupling. |
packages/react-components/react-headless-components-preview/library/bundle-isolation.config.json |
Extends bundle restrictions and debt allowance. |
packages/react-components/react-combobox/library/src/components/Dropdown/useDropdown.tsx |
Suppresses recorded Dropdown coupling. |
packages/react-components/react-combobox/library/src/components/Combobox/useCombobox.tsx |
Suppresses recorded Combobox coupling. |
packages/react-components/react-avatar/library/src/components/AvatarGroupPopover/useAvatarGroupPopover.tsx |
Records type-level motion coupling. |
packages/eslint-plugin/src/internal.js |
Configures the expanded runtime ban list. |
change/@fluentui-react-message-bar-9726a17c-0433-4787-b72b-90ba58ebfe61.json |
Records the MessageBar change. |
change/@fluentui-react-menu-639031a5-ca32-46af-9da2-9f57d8fc50d6.json |
Records the Menu change. |
change/@fluentui-react-headless-components-preview-9d9458fe-d4dc-4b37-90a0-e5e2c5fe7807.json |
Records the headless package change. |
change/@fluentui-react-combobox-55618c76-2dae-4993-a52c-d0269f1f96de.json |
Records the Combobox change. |
change/@fluentui-react-avatar-6892f968-8be8-4e9c-9fc6-b747d59db983.json |
Records the Avatar change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ca71dae to
029f215
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (4)
tools/eslint-rules/rules/base-hook-no-forbidden-runtime.ts:660
- Using a single
seenset with a depth limit can produce false negatives in shared type graphs. If a type is first reached at depth 10, its children are cut off at 11, but a later shallower path to that same type is skipped as already seen; a forbidden child that was within range of the shallower path is therefore never checked. Record the shallowest visited depth and revisit a type when the new path has more remaining depth.
if (budget-- <= 0 || depth > TYPE_WALK_MAX_DEPTH || seen.has(type)) {
return null;
}
seen.add(type);
packages/eslint-plugin/src/internal.js:32
- This list is not yet 1:1 with the bundle boundary's
@griffel/*ban:@griffel/shadow-domis an installed runtime package (yarn.lock:6766) but is absent here. A base hook can therefore reach it without a lint error even though bundle isolation rejects the same dependency. Add the known runtime satellite (or support the same package pattern semantics in the rule).
'@griffel/react',
'@griffel/core',
tools/eslint-rules/rules/base-hook-no-forbidden-runtime.ts:750
- The signature walk omits API-bearing signature elements. The constraint test passes only because
Tis reused as a value parameter, but<T extends HeavyOptions>() => voidhas no edge toT; similarly,(this: HeavyOptions) => voidand(value: unknown) => value is HeavyOptionsare missed because explicitthisparameters and predicate target types are not included ingetParameters()/getReturnType(). Traverse these signature types explicitly so forbidden coupling cannot hide in a valid public callback signature.
for (const signature of [...type.getCallSignatures(), ...type.getConstructSignatures()]) {
tools/eslint-rules/rules/base-hook-no-forbidden-runtime.ts:757
- Only the index value type is traversed, so coupling in the index key type is missed. For example, a public
{ [key: HeavyKey]: string }shape is treated as clean whenHeavyKeycomes from a forbidden package.IndexInfoexposes bothkeyTypeandtype; visit both.
for (const indexInfo of checker.getIndexInfosOfType(type)) {
const hit = visit(indexInfo.type, depth + 1);
Configure `base-hook-no-forbidden-runtime` explicitly with both `tabster` and `@fluentui/react-icons` instead of relying on the rule default, so the lint-time boundary matches the `forbiddenPackages` already asserted at build time by bundle-isolation.config.json. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e hooks Add `@fluentui/react-motion` and `@fluentui/react-motion-components-preview` to the base-hook forbidden runtimes and to the headless bundle-isolation `forbiddenPackages`. Both are workspace packages, which exposed a gap in the lint rule: ownership of a leaf declaration was derived from a `node_modules` path segment, but TypeScript resolves workspace packages through `paths` mappings straight to source, so a forbidden runtime reached through a collapsed re-export chain went unreported. Resolve ownership by walking up to the nearest named `package.json` instead, mirroring what verify-bundle-isolation already does, and cover it with a fixture that reproduces the workspace-package shape. Enabling the ban surfaced existing coupling, recorded as tracked debt: - runtime: `useMessageBarBase_unstable` calls `useMotionForwardedRef`, the only case that actually reaches the bundle (allowedViolations entry). - types: seven packages reference `PresenceMotionSlotProps` from their base hook signatures; type-only, so nothing is emitted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The type-position half of `base-hook-no-forbidden-runtime` walked declaration syntax, so it followed `ProgressBarBaseProps -> ProgressBarProps -> ProgressBarSlots -> MotionSlotProps` and reported coupling to `@fluentui/react-motion` that the resolved type does not have: the base types subtract exactly that member (`Omit<ProgressBarProps, 'indeterminateMotion'>`), and syntax cannot see the subtraction. Ask the checker instead. The resolved type is walked through its alias and declaration symbols, type arguments, union and intersection constituents, properties, signatures and index types, which makes the erased member simply absent. `aliasTypeArguments` are deliberately not followed, since that is where `Omit` keeps the pre-subtraction type. The walk skips members declared by the standard library and `@types` packages - they cannot reach a first-party runtime, and expanding them exhausted the traversal budget before real coupling was found. Value references keep the existing syntactic analysis, which is the right model for runtime dependencies. Across the v9 base hooks this turns 20 reports in 12 packages into 2 in one: only `AvatarGroupPopover` genuinely exposes motion, because its base types omit just `size` and keep the popover `root` slot. Those two stay recorded as debt; the other 18 suppressions are removed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add `@griffel/react` and `@griffel/core` so the lint boundary matches the `@griffel/*` glob the headless bundle-isolation config already asserts. The rule matches names exactly, so the glob has to be spelled out. Three base hooks compose styled building blocks that pull Griffel at runtime - the Combobox and Dropdown listbox slot, and Menu's safe-zone area. This is the same coupling the bundle config already allowlists, so it is recorded as debt rather than refactored here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ntime set The per-Program memo was keyed by symbol alone, but every answer it stores is only true relative to the `forbiddenRuntimes` set it was computed for. ESLint shares one TypeScript Program across every configuration pointed at the same tsconfig, so a result computed for one ban list could be served to another that bans different packages — reporting a runtime the second configuration explicitly allows, or reusing a `null` cached before the package was banned and missing a real violation. Partition the memo by a normalized key of the set. Configurations with identical ban lists still share a bucket, which is where the benefit of the cache comes from. The key is memoized per set instance so the sort stays off the per-symbol path. The repository configures the rule at a single site today, so this changes no current result; it is what keeps a second configuration from being wrong. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e walk The structural type walk bailed on anything that is not an object type, to keep primitives from expanding into their apparent members from the standard library. Type parameters, indexed accesses and conditionals are `Instantiable` rather than `Object`, so they were discarded by that same bail — taking their constraints with them. A constraint is part of the signature a base hook exposes: an imported callback typed `<T extends MotionSlotProps>(value: T) => void` couples the public API to the forbidden package even though `T` itself has no members. Resolve the base constraint before the primitive bail so that coupling is visible. Unconstrained parameters answer `undefined` and fall through unchanged, and the existing `seen` set and depth cap keep the extra edge bounded. Sweeping every package that declares a base hook produces no new violations, so this closes a blind spot without adding debt. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
029f215 to
d4bf67b
Compare
| "@fluentui/react-motion", | ||
| "@fluentui/react-motion-components-preview" | ||
| ], | ||
| "allowedViolations": {} |
There was a problem hiding this comment.
no violations !
Previous Behavior
The
base-hook-no-forbidden-runtimelint rule only guardedtabster, while the headlessverify-bundle-isolationconfig already assertedtabster,@griffel/*and@fluentui/react-icons. The two boundaries had drifted, so a v9 base hook could reach@fluentui/react-icons, the motion packages or Griffel without lint noticing.Two defects in the rule also surfaced while enabling the wider ban:
node_modulespath segment, but TypeScript resolves workspace packages throughtsconfigpathsstraight to source. A forbidden runtime reached through a collapsed re-export chain went unreported.ProgressBarBaseProps -> ProgressBarProps -> ProgressBarSlots -> MotionSlotPropsand reported coupling the resolved type does not have — the base types subtract exactly that member (Omit<ProgressBarProps, 'indeterminateMotion'>), and syntax cannot see a subtraction.New Behavior
Ban list now 1:1 with bundle verification.
@fluentui/react-icons,@fluentui/react-motion,@fluentui/react-motion-components-preview,@griffel/reactand@griffel/corejointabster, configured explicitly inpackages/eslint-plugin/src/internal.jsrather than relying on the rule default, so the lint-time boundary sits next to the build-timeforbiddenPackagesit mirrors. The rule matches names exactly, so satellites and the@griffel/*glob have to be spelled out.Workspace-aware ownership. Package identity is resolved by walking up to the nearest named
package.json, mirroring whatverify-bundle-isolationalready did. This also handles dependencies whose directory name differs from their declared name.Structural type analysis. Type references are now answered by the type checker — walking alias/declaration symbols, type arguments, union and intersection constituents, properties, signatures and index types — so an erased member is simply absent. Two subtleties are encoded in the implementation:
aliasTypeArgumentsare deliberately not followed: that is exactly whereOmit<T, K>retains the pre-subtraction type, so following it resurrects the erased member.Omit<...>whose alias symbol belongs tolib.es5.d.tseven though every member of the resolved shape is first-party.Value references keep the existing syntactic analysis, which is the right model for runtime dependencies.
On the motion ban alone this turns 20 reports in 12 packages into 2 in one — Accordion, Drawer, Menu, Nav, Progress and TeachingPopover were all false positives.
Coupling found and recorded as debt
All of it pre-existing. Note that the two checks share a package list but cover different surfaces: the lint rule inspects
use*Base_unstablehooks wherever they are declared, while bundle verification measures what survives into the headless entry points. Only MessageBar is common to both — the headless package consumesuseMessageBarBase_unstabledirectly, which is exactly why@fluentui/react-motionshows up in the bundle report. The Griffel rows below are source-level only today: the headless package reimplementsuseMenu/useComboboxand borrows justMENU_ENTER_EVENTanduseInputTriggerSlot, so those chains do not currently reach a bundle. (Griffel used to leak into the bundle through@fluentui/react-portal'susePortalMountNode; #36553 has since closed that onmaster, so after rebasing,@fluentui/react-motionvia MessageBar is the only bundle-level debt left — and #36549 removes that one too.)@fluentui/react-motionuseMessageBarBase_unstablecallsuseMotionForwardedRef— the only case that actually reaches the bundle@fluentui/react-motionAvatarGroupPopoverbase types omit onlysize, keeping the popoverrootslot and with itsurfaceMotion@griffel/reactuseComboboxBase_unstable/useDropdownBase_unstablecompose the styledListboxslot@griffel/reactuseMenuBase_unstablecallsuseSafeZoneArea, which renders<SafeZoneArea>; that component callsmergeClassesand its styles module callsmakeStylesat module scope@fluentui/react-iconsand@fluentui/react-motion-components-previewcame back completely clean — nothing to record.Validation
eslint-rulestests pass, including new fixtures for the workspace-package shape, anOmit-erased / not-erased pair, and a coupled / clean type-parameter-constraint pair.verify-bundle-isolation:PASS WITH DEBT — 3 fixtures, 0 regressions, 1 allowed violation,kept out: tabster, @griffel/*, @fluentui/react-icons, @fluentui/react-motion-components-preview. The single allowlist entry is@fluentui/react-motionviauseMessageBarBase_unstable; TagPicker and TeachingPopover areCLEAN.beachball checkpasses; change files aretype: nonesince every change to a published package is comments or lint config only.Review follow-ups
Two further rule defects were raised in review and are fixed here as separate commits:
forbiddenRuntimes. ESLint shares one TypeScript Program across configurations pointed at the same tsconfig, so a result computed for one ban list could be served to another that bans different packages. Reproduced (a config banning onlyworkspace-runtimewas told aboutheavy-runtime), then fixed by partitioning the memo by a normalized key of the set. No current result changes — the repo configures the rule at one site — so this is insurance against a second configuration.Instantiabletypes are notObject, so they were discarded by the bail that keeps primitives from expanding into their standard-library members — taking their constraints with them. An imported<T extends MotionSlotProps>(value: T) => voidwas reported clean despite exposing the forbidden package in its signature. Now resolved viagetBaseConstraintOfTypebefore the bail. This makes the rule stricter, so the whole-repo sweep was re-run: still 0 violations, 0 unused suppressions.A third comment — that the type walk resolves the imported symbol rather than the type at the reference site, so an inline
props: Omit<StyledProps, 'motion'>still reports the erased member — is accurate and deliberately not fixed here. The erasure is handled when written as a declaration (the ProgressBar case above) but not inline. I implemented the fix, got it green, and dropped it: it was ~83% of the change's complexity and 7 new functions, surfaced five separate precision defects (four only after an earlier version passed the full suite), and addresses a pattern that occurs zero times across the 39 packages declaring a base hook. Details in the review thread; it can land as its own reviewable change if the pattern appears.Notes for reviewers
Related Issue(s)