perf(useSlots): optimize slot matching with short-circuit and reduced allocations#7555
Conversation
🦋 Changeset detectedLatest commit: ef3c4aa 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 |
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
91c8339 to
4a250c7
Compare
4a250c7 to
c924742
Compare
c924742 to
a1d1272
Compare
fe1f7f0 to
8f048b0
Compare
There was a problem hiding this comment.
Pull request overview
This PR optimizes the useSlots hook in @primer/react, which is used widely across components to extract “slot” children while remaining SSR-compatible. The changes focus on reducing per-render allocations and enabling faster matching in common cases.
Changes:
- Replaces higher-order utilities (
mapValues,findIndex) withforloops to reduce allocations and overhead. - Adds a “all slots filled” short-circuit path to avoid further matching work.
- Adds/extends unit tests for additional slot behaviors and introduces a changeset for a patch release.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react/src/hooks/useSlots.ts | Refactors slot initialization/matching logic and adds a short-circuit path + helper functions. |
| packages/react/src/hooks/tests/useSlots.test.tsx | Adds tests for short-circuit behavior, non-element children handling, and slot order independence. |
| .changeset/perf-use-slots-short-circuit.md | Declares a patch release entry for the perf change. |
| // Short-circuit: all slots filled, no more matching needed | ||
| if (slotsFound === totalSlots) { | ||
| if (__DEV__ && warnIfDuplicate(child, keys, values, totalSlots)) { | ||
| return | ||
| } | ||
| }) | ||
|
|
||
| // If the child is not a slot, add it to the `rest` array | ||
| if (index === -1) { | ||
| rest.push(child) | ||
| return |
There was a problem hiding this comment.
In production builds (DEV is false), the short-circuit path pushes all subsequent elements into rest without checking whether they match a slot. This changes behavior vs the previous implementation: duplicate slot children that appear after all slots are filled will now be treated as rest (and can be rendered), even though the warning message/semantics say duplicates are ignored (“Only the first will be rendered.”). To preserve prior behavior, avoid adding children that match any slot to rest even in the short-circuit path (you can still keep it cheap by first checking child.type against a precomputed set of configured component matchers before running the full matcher/testFn logic).
| */ | ||
| function warnIfDuplicate( | ||
| child: React.ReactElement, | ||
| keys: Array<string | number | symbol>, |
There was a problem hiding this comment.
warnIfDuplicate takes keys: Array<string | number | symbol>, but keys is produced via Object.keys(config) and cast to Array<keyof Config> (effectively strings). Tightening this helper signature to Array<keyof Config> (or string[]) would better reflect reality and avoid type widening that can mask mistakes.
| keys: Array<string | number | symbol>, | |
| keys: string[], |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/14138 |
Closes #
Optimizes the
useSlotshook used by 8+ components (ActionList.Item, ActionList, FormControl, TreeView, NavList, PageLayout, Dialog, CheckboxOrRadioGroup). The hook iterates children on every render to extract slot components.Changes:
restwith zero type checking (single integer comparison). Duplicate detection only runs in__DEV__and is stripped in production builds.forloop instead offindIndex: eliminates closure allocation per child per renderforloop init instead ofmapValues+Object.keys().reduce(): replaces a higher-orderreducewith a simple loop to initialize slot keysMetrics
Microbenchmark (Chrome, 100K iterations, JIT-warmed):
mapValues+reducevsforloop)findIndex+closure vsforloop)useSlotscallStress test (ActionList, 100 items x 100 re-renders):
Real-world impact is larger than this test shows: 8+ components call
useSlots, production builds strip the__DEV__duplicate-detection loop.Changelog
New
N/A
Changed
useSlots: optimized child iteration with production short-circuit and reduced allocationsRemoved
N/A
Rollout strategy
Testing & Reviewing
useSlotsunit tests pass unmodified (behavior-preserving)Merge checklist