From 1001907933984e8bc4d2127a55cf3a2145991c3f Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Fri, 22 May 2026 20:59:07 +0000 Subject: [PATCH 1/6] feat(internal): add useDevOnlyEffect hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wraps useEffect with a build-time __DEV__ check. Lets consumers replace the inline 'if (__DEV__) useEffect(...)' pattern that previously required an eslint-disable-next-line react-hooks/rules-of-hooks comment at every site — the conditional now lives inside a regular hook, satisfying the linter and the React Compiler's heuristics. The internal __DEV__ guard keeps the hook safe under any build pipeline that doesn't apply the strip plugin (storybook, unit tests, third parties); upcoming commits add a babel plugin and a dual rollup build that strip every useDevOnlyEffect call statement from the production dist artifact. --- .../src/internal/hooks/useDevOnlyEffect.ts | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 packages/react/src/internal/hooks/useDevOnlyEffect.ts diff --git a/packages/react/src/internal/hooks/useDevOnlyEffect.ts b/packages/react/src/internal/hooks/useDevOnlyEffect.ts new file mode 100644 index 00000000000..c153c62c7f2 --- /dev/null +++ b/packages/react/src/internal/hooks/useDevOnlyEffect.ts @@ -0,0 +1,37 @@ +import {useEffect} from 'react' + +/** + * Runs an effect only in development. In production builds the call is + * stripped at publish time by `script/babel-plugin-strip-dev-only-effect.cjs`, + * which removes every `useDevOnlyEffect(...)` call statement from the + * `dist/production/` artifact. The development artifact keeps the call as-is. + * + * `package.json`'s `exports` field selects between the two artifacts via the + * `development` / `production` import conditions, so consumers in development + * see the assertions and consumers in production pay zero bytes — no call, no + * effect closure, and no deps array. + * + * Implementation notes: + * + * - The internal `if (__DEV__)` guard is a defensive fallback for build + * pipelines that don't apply the strip plugin (e.g. the source consumed by + * storybook / unit tests, which set `__DEV__: true`). It keeps the call + * semantically equivalent to the inline `if (__DEV__) { useEffect(...) }` + * pattern that previously needed an `eslint-disable react-hooks/rules-of-hooks` + * comment at every site. By moving the guard inside a regular hook we satisfy + * the Rules of Hooks linter and the React Compiler's heuristics, neither of + * which can prove that `__DEV__` is build-time constant. + * + * - The hook itself is not conditionally invoked by callers, so it never + * violates Rules of Hooks at runtime: in dev `__DEV__` is `true`, in prod + * the call site has been removed entirely. + * + * @param effect The effect callback to run in development. + * @param deps Optional dependency list, same semantics as `useEffect`. + */ +export const useDevOnlyEffect = (effect: React.EffectCallback, deps?: React.DependencyList) => { + if (__DEV__) { + // eslint-disable-next-line react-hooks/rules-of-hooks + useEffect(effect, deps) + } +} From 2cb594e5cbcbb486d1b0f551259dcce1adf053b2 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Fri, 22 May 2026 21:03:22 +0000 Subject: [PATCH 2/6] refactor(react): migrate dev-only effects to useDevOnlyEffect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every existing 'if (__DEV__) { useEffect(...) }' block had an 'eslint-disable-next-line react-hooks/rules-of-hooks' comment because the linter couldn't prove __DEV__ was build-time constant. Replace those four sites — UnderlineNav, Heading, Link, Banner — with the new useDevOnlyEffect hook, which moves the conditional inside a regular hook and satisfies Rules of Hooks without an eslint-disable. Behavior is unchanged: useDevOnlyEffect internally guards its useEffect call with the same 'if (__DEV__)' check, and __DEV__ is replaced with 'false' in production builds (the existing transform-replace-expressions pipeline). Stripping the call site entirely from the production dist artifact — including the effect closure and deps array — is the follow-up work in the next commits. --- packages/react/src/Banner/Banner.tsx | 41 +++++++++---------- packages/react/src/Heading/Heading.tsx | 24 ++++------- packages/react/src/Link/Link.tsx | 40 ++++++++---------- .../react/src/UnderlineNav/UnderlineNav.tsx | 21 ++++------ 4 files changed, 52 insertions(+), 74 deletions(-) diff --git a/packages/react/src/Banner/Banner.tsx b/packages/react/src/Banner/Banner.tsx index 8837134b86d..0fb19589f75 100644 --- a/packages/react/src/Banner/Banner.tsx +++ b/packages/react/src/Banner/Banner.tsx @@ -1,5 +1,6 @@ import {clsx} from 'clsx' -import React, {forwardRef, useEffect} from 'react' +import React, {forwardRef} from 'react' +import {useDevOnlyEffect} from '../internal/hooks/useDevOnlyEffect' import {AlertIcon, InfoIcon, StopIcon, CheckCircleIcon, XIcon} from '@primer/octicons-react' import {Button, IconButton, type ButtonProps} from '../Button' import {VisuallyHidden} from '../VisuallyHidden' @@ -132,27 +133,23 @@ export const Banner = React.forwardRef(function Banner const visual = leadingVisual ?? icon - if (__DEV__) { - // This hook is called consistently depending on the environment - // eslint-disable-next-line react-hooks/rules-of-hooks - useEffect(() => { - if (title) { - return - } - - const {current: banner} = bannerRef - if (!banner) { - return - } - - const hasTitle = banner.querySelector('[data-banner-title]') - if (!hasTitle) { - throw new Error( - 'Expected a title to be provided to the component with the `title` prop or through `` but no title was found', - ) - } - }, [title]) - } + useDevOnlyEffect(() => { + if (title) { + return + } + + const {current: banner} = bannerRef + if (!banner) { + return + } + + const hasTitle = banner.querySelector('[data-banner-title]') + if (!hasTitle) { + throw new Error( + 'Expected a title to be provided to the component with the `title` prop or through `` but no title was found', + ) + } + }, [title]) return ( diff --git a/packages/react/src/Heading/Heading.tsx b/packages/react/src/Heading/Heading.tsx index 88b8e31a252..3a0b4a85ad6 100644 --- a/packages/react/src/Heading/Heading.tsx +++ b/packages/react/src/Heading/Heading.tsx @@ -1,5 +1,6 @@ import {clsx} from 'clsx' -import React, {forwardRef, useEffect} from 'react' +import React, {forwardRef} from 'react' +import {useDevOnlyEffect} from '../internal/hooks/useDevOnlyEffect' import {useMergedRefs} from '../hooks' import type {ComponentProps} from '../utils/types' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' @@ -16,21 +17,12 @@ const Heading = forwardRef(({as: Component = 'h2', className, variant, ...props} const innerRef = React.useRef(null) const mergedRef = useMergedRefs(forwardedRef, innerRef) - 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 - useEffect(() => { - if (innerRef.current && !(innerRef.current instanceof HTMLHeadingElement)) { - // eslint-disable-next-line no-console - console.warn('This Heading component should be an instanceof of h1-h6') - } - }, [innerRef]) - } + useDevOnlyEffect(() => { + if (innerRef.current && !(innerRef.current instanceof HTMLHeadingElement)) { + // eslint-disable-next-line no-console + console.warn('This Heading component should be an instanceof of h1-h6') + } + }, [innerRef]) return ( ( const innerRef = React.useRef>(null) const mergedRef = useMergedRefs(ref, innerRef) - 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 - useEffect(() => { - if ( - innerRef.current && - !(innerRef.current instanceof HTMLButtonElement) && - !(innerRef.current instanceof HTMLAnchorElement) - ) { - // eslint-disable-next-line no-console - console.error( - 'Error: Found `Link` component that renders an inaccessible element', - innerRef.current, - 'Please ensure `Link` always renders as or