Skip to content

fix(react): enable React Compiler for hooks#8124

Closed
joshblack wants to merge 2 commits into
mainfrom
copilot/react-compiler-hooks-v2
Closed

fix(react): enable React Compiler for hooks#8124
joshblack wants to merge 2 commits into
mainfrom
copilot/react-compiler-hooks-v2

Conversation

@joshblack

Copy link
Copy Markdown
Member

Follow up to the React Compiler migration work. This PR enables React Compiler for the remaining hook utilities under packages/react/src/hooks.

Changelog

New

  • Add an internal dependency-list effect helper used by hook utilities that expose dependency-list APIs.

Changed

  • Update hook utilities to avoid React hook-rule suppressions while preserving the existing public hook APIs.
  • Remove hook files from the React Compiler unsupported list.

Removed

  • Remove React Compiler exclusions for useAnchoredPosition, useFocusTrap, useFocusZone, useMenuInitialFocus, useOnEscapePress, useResizeObserver, and useSafeTimeout.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

This keeps the published API the same and updates our internal compiler compatibility for hooks.

Testing & Reviewing

Would love a review on the dependency tracking changes in the hook utilities, especially useFocusTrap and useAnchoredPosition, since those preserve existing dependency-list behavior while avoiding React Compiler skips.

Verified with:

  • npm run build
  • npm test -- --run
  • npm run type-check
  • npm run lint
  • npm run lint:css
  • npm run format:diff
  • packages/react/node_modules/.bin/tsx script/react-compiler-migration-status.mts
  • Direct React Compiler transform check for the enabled hook files

Merge checklist

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@changeset-bot

changeset-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: f9a6462

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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 github-actions Bot added staff Author is a staff member integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Jul 6, 2026
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

⚠️ Action required

👋 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. If this doesn't work, you can also use the original workflow here. Check the integration testing docs for step-by-step instructions. Or, apply the integration-tests: skipped manually label to skip these checks.

To publish a canary release for integration testing, apply the Canary Release label to this PR.

Comment thread .changeset/react-compiler-hooks.md Outdated
'@primer/react': patch
---

Hooks: Enable React Compiler optimization for hook utilities

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot be specific about the hooks changed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 73cd1b5 — changeset now lists each hook by name.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot I'm not a big fan of this approach. Let's find something better

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally open to a different approach here. The core problem is that these hooks expose a caller-supplied DependencyList which React Compiler can't track statically, so I reached for a manual comparison wrapper. What direction were you thinking? For example: dropping the dep-list override entirely, converting to a callback-based API, or something else? Happy to implement once the direction is clear.

Co-authored-by: joshblack <3901764+joshblack@users.noreply.github.com>
@github-actions github-actions Bot temporarily deployed to storybook-preview-8124 July 6, 2026 20:32 Inactive
@joshblack

Copy link
Copy Markdown
Member Author

@copilot I'm not a big fan of the useDependencyList approach. Let's try something else

Copilot AI commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

@copilot I'm not a big fan of the useDependencyList approach. Let's try something else

Happy to change direction. Before I implement something, which of these would you prefer?

  1. Inline the comparison logic — replace useDependencyListEffect with a useRef-based comparison directly inside each hook (same semantics, no shared abstraction)
  2. Stable callback pattern — instead of accepting a DependencyList, these hooks accept a () => void effect callback (caller stabilizes it with useCallback); the hook just calls it in a regular useEffect([callback])
  3. Remove the dependencies parameter — drop it from the public API and always compute dependencies inside the hook (potential behavior change for existing callers)
  4. Keep the eslint-disable pattern and leave these hooks in the React Compiler exclusion list — revert to the original code, just not compiler-compatible yet

Or if you have a different approach in mind, let me know and I'll implement that.

@joshblack joshblack closed this Jul 6, 2026
@primer primer Bot deleted the copilot/react-compiler-hooks-v2 branch July 7, 2026 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm staff Author is a staff member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants