perf(Text): remove unnecessary useRef and useImperativeHandle hooks#7551
perf(Text): remove unnecessary useRef and useImperativeHandle hooks#7551hectahertz wants to merge 7 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 2f71900 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 |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the Text component in @primer/react by removing internal ref plumbing (useRef + useImperativeHandle) and forwarding the received ref directly to the rendered element, reducing per-render hook usage for a frequently-rendered component.
Changes:
- Remove
useRef/useRefObjectAsForwardedReffromTextand attach the forwardedrefdirectly to the polymorphic element. - Add a patch changeset documenting the performance-focused change.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/react/src/Text/Text.tsx | Removes internal ref hooks and forwards ref directly to the rendered element. |
| .changeset/text-remove-hooks.md | Patch changeset entry describing the perf change. |
Comments suppressed due to low confidence (1)
packages/react/src/Text/Text.tsx:22
- Ref-forwarding behavior changed (ref now passed directly to the rendered element), but the existing Text unit tests don’t assert ref behavior. Add a test that renders (and ideally as well) and verifies ref.current points at the underlying DOM element to prevent regressions.
<Component className={clsx(className, classes.Text)} data-size={size} data-weight={weight} {...rest} ref={ref} />
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/14068 |
|
Integration test results from github/github-ui:
CI check runs linting, type checking, and unit tests. Check the workflow logs for specific failures. Need help? If you believe this failure is unrelated to your changes, please reach out to the Primer team for assistance. |
Closes #
Text had
useRef+useRefObjectAsForwardedRef(wrappinguseImperativeHandle) just to forward a ref that was never read internally. Removed both hooks and passed the forwarded ref directly to the element.useRefuseImperativeHandle(viauseRefObjectAsForwardedRef)Text is one of the most frequently rendered Primer components. On a typical page with 50-100 Text instances, this eliminates ✨ 100-200 hook calls per render cycle ✨.
Changelog
New
N/A
Changed
Removed
N/A
Rollout strategy
Testing & Reviewing
<Text ref={myRef}>Hello</Text>and verifymyRef.currentpoints to the DOM spanasprop still works:<Text as="p" ref={myRef}>Hello</Text>sizeandweightprops still apply data attributesMerge checklist