perf(Button): fix CounterLabel remount, remove DEV hook, enable React Compiler#7552
Conversation
🦋 Changeset detectedLatest commit: ac1a16c 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 |
github-actions
bot
added
the
integration-tests: recommended
|
👋 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 |
github-actions
bot
requested a deployment
to
storybook-preview-7552
hectahertz
changed the title
There was a problem hiding this comment.
Pull request overview
Optimizes Button rendering in @primer/react by preventing unnecessary CounterLabel remounts, removing a DEV-only conditional hook to allow React Compiler optimization, and updating the compiler support list accordingly.
Changes:
- Pass
CounterLabelas a JSX element torenderModuleVisualto avoid unmount/remount churn on re-renders. - Replace the DEV-only conditional
useEffectwith a render-time element validation check. - Enable React Compiler for Button by removing
src/Button/**/*.tsxfrom the unsupported list, and add a patch changeset.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react/src/Button/ButtonBase.tsx | Fixes CounterLabel rendering to avoid remounts; changes DEV validation logic. |
| packages/react/script/react-compiler.mjs | Removes Button from React Compiler unsupported set. |
| .changeset/button-optimize-render.md | Adds patch changeset describing the Button perf change. |
Comments suppressed due to low confidence (1)
packages/react/src/Button/ButtonBase.tsx:158
- There doesn’t appear to be unit test coverage for the
count/CounterLabel rendering path, and this change is specifically meant to prevent CounterLabel being unmounted/remounted across re-renders. Please add a regression test that renders a Button withcount, re-renders with a differentcount, and asserts the CounterLabel DOM node identity is preserved (no remount) while its text updates.
count !== undefined && !TrailingVisual
? renderModuleVisual(
<CounterLabel className={classes.CounterLabel} data-component="ButtonCounter">
{count}
</CounterLabel>,
Boolean(loading) && !LeadingVisual,
'trailingVisual',
true,
)
| if (__DEV__) { | ||
| /** | ||
| * The Linter yells because it thinks this conditionally calls an effect, | ||
| * but since this is a compile-time flag and not a runtime conditional | ||
| * this is safe, and ensures the entire effect is kept out of prod builds | ||
| * shaving precious bytes from the output, and avoiding mounting a noop effect | ||
| */ | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| React.useEffect(() => { | ||
| if ( | ||
| innerRef.current && | ||
| !(innerRef.current instanceof HTMLButtonElement) && | ||
| !((innerRef.current as unknown) instanceof HTMLAnchorElement) && | ||
| !((innerRef.current as HTMLElement).tagName === 'SUMMARY') | ||
| ) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn('This component should be an instanceof a semantic button or anchor') | ||
| } | ||
| }, [innerRef]) | ||
| // Validate that the element is a semantic button/anchor. | ||
| // This runs during render (not in an effect) to avoid a conditional hook call | ||
| // that prevents React Compiler from optimizing this component. | ||
| const el = innerRef.current | ||
| if ( | ||
| el && | ||
| !(el instanceof HTMLButtonElement) && | ||
| !((el as unknown) instanceof HTMLAnchorElement) && | ||
| !((el as HTMLElement).tagName === 'SUMMARY') | ||
| ) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn('This component should be an instanceof a semantic button or anchor') | ||
| } |
There was a problem hiding this comment.
The new DEV validation runs during render and uses HTMLButtonElement/HTMLAnchorElement. In SSR/non-DOM environments those globals can be undefined, causing a ReferenceError when __DEV__ is true (breaking SSR compatibility). Consider moving this check into an unconditional (isomorphic) effect with the __DEV__ check inside, or guarding with canUseDOM / typeof HTMLButtonElement !== 'undefined' before doing any instanceof checks.
| !((el as HTMLElement).tagName === 'SUMMARY') | ||
| ) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn('This component should be an instanceof a semantic button or anchor') |
There was a problem hiding this comment.
The warning message text is grammatically incorrect ("an instanceof a"). Since this is user-facing console output in dev, it should be updated to something clearer like "should be an instance of a semantic button or anchor" (and ideally include the actual rendered tag name).
| console.warn('This component should be an instanceof a semantic button or anchor') | |
| console.warn( | |
| `ButtonBase should render a semantic <button> or <a> element, but rendered <${(el as HTMLElement).tagName.toLowerCase()}> instead.`, | |
| ) |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/13891 |
2 participants
Closes #
Three optimizations for Button, the most frequently used component, plus enabling React Compiler.
1. Fix CounterLabel unmount/remount on every re-render
An inline arrow function was passed as a component type to
renderModuleVisual. React saw a new component reference every render, unmounting and remounting the CounterLabel DOM subtree instead of diffing the text content in place. Now passes CounterLabel as a JSX element directly, soisElement()returns true and it renders via identity-stable{Visual}instead of<Visual />.2. Remove conditional
__DEV__hookA
React.useEffectinside anif (__DEV__)block was the primary reason Button was excluded from React Compiler. Replaced with a synchronous render-time ref check that achieves the same validation without a hook call.3. Enable React Compiler for Button
With the conditional hook removed, Button is fully compatible with React Compiler. Removed
src/Button/**/*.tsxfrom the exclusion list.Measurements (500 Buttons with changing
countprop)On main, the inline arrow function causes React to remove and reinsert each CounterLabel
<span>on every re-render (1,000 DOM operations). With the fix, React diffs the text content in place with zero node churn.Changelog
New
N/A
Changed
Removed
src/Button/**/*.tsxfrom the React Compiler exclusion listRollout strategy
Testing & Reviewing
<Button count={5}>Click</Button>and verify the counter appears<Button as="div">test</Button>and verify console warning still appearsnpx vitest run packages/react/src/Button/Merge checklist