Skip to content

feat(react): dual dist build strips dev-only effects from production#7874

Draft
mattcosta7 wants to merge 6 commits into
mainfrom
feat/dev-only-effect-dual-build
Draft

feat(react): dual dist build strips dev-only effects from production#7874
mattcosta7 wants to merge 6 commits into
mainfrom
feat/dev-only-effect-dual-build

Conversation

@mattcosta7
Copy link
Copy Markdown
Contributor

Closes #

Summary

Replaces every if (__DEV__) { useEffect(...) } block that needed an
eslint-disable react-hooks/rules-of-hooks comment with a new internal
useDevOnlyEffect hook, and pairs it with a dual dist/development/ +
dist/production/ build so the production artifact ships zero bytes for
every dev-only assertion site — no call, no effect-callback closure, no deps
array, and the hook implementation itself is tree-shaken away.

The public API is unchanged. Consumers' bundlers automatically pick the right
artifact via package.json's exports development / production import
conditions; no consumer code changes are required.

Motivation

Four components — UnderlineNav, Heading, Link, and Banner — were
performing dev-only validations through this pattern:

if (__DEV__) {
  // The Linter yells because it thinks this conditionally calls an effect…
  // eslint-disable-next-line react-hooks/rules-of-hooks
  useEffect(() => {
    invariant(/* … */)
  })
}

Two problems:

  1. react-hooks/rules-of-hooks is disabled at every call site. The linter
    can't prove __DEV__ is build-time constant, and the React Compiler bails
    on the file because of the inline disable. Every new dev-only effect would
    need the same boilerplate, with the same risk of someone reading it as
    "hooks may be called conditionally."

  2. The closure body ships in dist/. With the existing single-artifact
    build, __DEV__ is rewritten to process.env.NODE_ENV !== 'production',
    so the published bundle contains:

    if (process.env.NODE_ENV !== 'production') {
      useEffect(() => { /* full assertion body */ }, [deps])
    }

    The consumer's bundler eventually DCEs that in production builds, but the
    closure body is in the npm package on every install, and bundlers that
    don't do iterative DCE leave the empty if block plus the closure
    allocation in the output.

This PR fixes both — the eslint-disable goes away from consumer sites, and
the production artifact ships nothing.

What changed

1. New useDevOnlyEffect hook (packages/react/src/internal/hooks/useDevOnlyEffect.ts)

export const useDevOnlyEffect = (effect: React.EffectCallback, deps?: React.DependencyList) => {
  if (__DEV__) {
    // eslint-disable-next-line react-hooks/rules-of-hooks
    useEffect(effect, deps)
  }
}

The one remaining eslint-disable lives inside the hook itself, where it's a
documented invariant: the hook is always called unconditionally by consumers,
so Rules of Hooks is satisfied at runtime in any build. The internal __DEV__
guard is a defensive fallback for build pipelines (storybook, unit tests,
third-party consumers without our babel plugin) where __DEV__ is true.

2. Migrate the four call sites

Every eslint-disable-next-line react-hooks/rules-of-hooks comment in
consumer code is gone; the linter and the React Compiler are both happy.
Behavior is identical in development.

3. babel-plugin-strip-dev-only-effect (packages/react/script/babel-plugin-strip-dev-only-effect.cjs)

Removes every useDevOnlyEffect(...) call statement whose identifier resolves
to an import from a useDevOnlyEffect module. Conservative matching:

  • Only direct identifier calls; aliased imports left alone.
  • Only when the binding's import source includes useDevOnlyEffect.
  • Only statement-position calls (hooks aren't valid expressions).

After this runs, the call statement, its effect-callback closure, and its
deps array are all gone from the AST. The useDevOnlyEffect import binding
typically becomes unused; rollup's tree-shaking drops it next.

4. Dual rollup build (packages/react/rollup.config.mjs)

Refactored into a makeConfig(mode) factory exporting two configurations:

Mode Output __DEV__ Strip plugin
development dist/development/ true (literal) not applied
production dist/production/ false (literal) applied

A subtle ordering fix: babel-plugin-transform-replace-expressions now runs
before babel-plugin-dev-expression. The latter rewrites __DEV__ to a
runtime process.env.NODE_ENV !== 'production' check; running our literal
swap first lets rollup constant-fold the resulting if (true) { … } /
if (false) { … } blocks during bundling.

5. package.json exports (packages/react/package.json)

Every public entry (., ./experimental, ./deprecated, ./next,
./test-helpers) gains development and production conditions:

"./experimental": {
  "types": "./dist/experimental/index.d.ts",
  "development": "./dist/development/experimental/index.js",
  "production": "./dist/production/experimental/index.js",
  "default": "./dist/production/experimental/index.js"
}

main/module default to the production artifact for tools that don't
honour exports conditions. Type declarations (.d.ts) stay at the dist
root — they're identical for both artifacts, so there's no value in
duplicating them.

End-to-end verification

Real consumer build with esbuild + --minify + NODE_ENV='production':

useDevOnlyEffect references in prod bundle:     0
assertion message strings ('Only one current
   element is allowed' etc.) in prod bundle:    0
process.env.NODE_ENV references (substituted):  0

Same setup in development mode (NODE_ENV='development', no --minify):

useDevOnlyEffect references in dev bundle:      6
assertion message strings in dev bundle:        1+

In our own dist:

dist/development/internal/hooks/useDevOnlyEffect.js
  → const useDevOnlyEffect = (effect, deps) => {
      { useEffect(effect, deps); }    // 'if (__DEV__)' folded to bare block
    }

dist/development/UnderlineNav/UnderlineNav.js
  → useDevOnlyEffect(() => { /* full assertions */ }, …)

dist/production/UnderlineNav/UnderlineNav.js
  → (call site, closure, and deps array entirely removed)

dist/production/internal/hooks/useDevOnlyEffect.js
  → (file does not exist — rollup tree-shook it because no caller references it)

Changelog

New

  • Internal useDevOnlyEffect hook for dev-only side effects without
    eslint-disable react-hooks/rules-of-hooks.
  • babel-plugin-strip-dev-only-effect build plugin.
  • Dual dist/development/ and dist/production/ published artifacts.
  • development and production import conditions on every public exports
    entry.

Changed

  • UnderlineNav, Heading, Link, and Banner use useDevOnlyEffect
    instead of an inline if (__DEV__) { useEffect(...) } block. No behavioural
    change.
  • package.json main/module now point at ./dist/production/index.js
    (was ./dist/index.js). Consumers using exports conditions are unaffected.

Removed

  • Four eslint-disable-next-line react-hooks/rules-of-hooks comments at
    consumer sites (UnderlineNav, Heading, Link, Banner).

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

Public API is unchanged. Existing consumers automatically benefit from a
smaller production bundle on upgrade. The only theoretically observable
change is the dist directory layout — deep imports into
@primer/react/dist/... (which are off-spec and outside the exports
contract) will need to add development/ or production/ to their path. We
don't sanction such imports.

Testing & Reviewing

  • All 72 existing tests for the four migrated components pass unchanged.
  • tsc --noEmit clean; eslint --max-warnings=0 clean for the touched
    files.
  • Full npm run build succeeds end-to-end (rollup → tsc).
  • Verified empirically with esbuild that the production artifact strips
    every useDevOnlyEffect reference, closure body, deps array, and the
    hook's own module from the consumer's prod bundle.

To verify locally:

cd packages/react
rm -rf dist
npx rollup -c                           # produces both artifacts
ls dist/development dist/production     # confirm dual layout

# Strip plugin actually removes the call:
grep -c useDevOnlyEffect \
  dist/development/UnderlineNav/UnderlineNav.js \
  dist/production/UnderlineNav/UnderlineNav.js
# expected: development has 2 (import + call), production has 0

Reviewer focus:

  1. useDevOnlyEffect's comment block — does the invariant chain
    (eslint-disable inside hook + plugin strip at publish + dev fallback)
    make sense in five years?
  2. The babel-plugin ordering change (transform-replace-expressions before
    dev-expression) — does that affect any other dev-expression-rewritten
    patterns we care about? invariant and warning calls still pass
    through unchanged; the only thing that changes is __DEV__ resolution.
  3. package.json exports — is default → production the right fallback
    for tools that don't honour conditions?

Merge checklist

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.
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.
Removes every useDevOnlyEffect(...) call statement whose identifier
resolves to an import from a useDevOnlyEffect module. To be used only by
the production rollup build — the development build keeps the calls and
relies on __DEV__: true to make them run.

After this plugin runs, the call statement and its arguments (effect
callback closure and deps array) are entirely gone. The useDevOnlyEffect
import binding may then be unused; rollup tree-shaking drops it and, in
turn, drops the useDevOnlyEffect module from the bundle.

Conservative matching:
  - Only direct identifier calls; aliased imports are ignored.
  - Only when the binding's import source includes 'useDevOnlyEffect'.
  - Only statement-position calls (hooks aren't valid expressions).
…acts

Refactor rollup.config.mjs into a makeConfig(mode) factory and export two
configurations: one for 'development' that emits to dist/development/ and
one for 'production' that emits to dist/production/.

Differences between the two:

  - Both run the same base babel pipeline (react-compiler, macros,
    add-react-displayname, dev-expression, styled-components, etc.).
  - Production additionally runs babel-plugin-strip-dev-only-effect,
    which removes every useDevOnlyEffect(...) call statement from the
    output before the __DEV__ swap.
  - babel-plugin-transform-replace-expressions is reordered to run BEFORE
    dev-expression so that the __DEV__ identifier is replaced with the
    real boolean literal ('true' in development, 'false' in production)
    rather than dev-expression's runtime check on
    process.env.NODE_ENV. Rollup then constant-folds the resulting
    'if (true)' / 'if (false)' blocks during bundling.

Result, for a representative dev-only assertion site:

  dist/development/UnderlineNav/UnderlineNav.js
    useDevOnlyEffect(() => { ...assertions... })

  dist/development/internal/hooks/useDevOnlyEffect.js
    const useDevOnlyEffect = (effect, deps) => {
      { useEffect(effect, deps); }    // 'if (__DEV__)' folded away
    }

  dist/production/UnderlineNav/UnderlineNav.js
    (assertion site removed entirely — closure and deps array gone)

  dist/production/internal/hooks/useDevOnlyEffect.js
    (file does not exist — tree-shaken because no caller references it)

Next commit wires package.json's 'exports' field with development /
production import conditions so consumers' bundlers automatically pick
the right artifact.
Updates package.json's exports field to declare 'development' and
'production' import conditions for every public entry (root, experimental,
deprecated, next, test-helpers). Consumers' bundlers pick the matching
artifact based on their build mode:

  - Development bundlers see ./dist/development/<entry>.js — full
    useDevOnlyEffect call sites, all dev-only invariants live.
  - Production bundlers see ./dist/production/<entry>.js — useDevOnlyEffect
    call sites and effect closures removed by the strip plugin at our
    build time, before publish.

Sets main / module to ./dist/production/index.js as the safer default for
tools that don't honor exports conditions (rare, but cheaper to ship dev
assertions hidden behind dead branches than to expose them).

typings stays at ./dist/index.d.ts — tsc still emits a single shared set
of declarations to the dist root (declarations are identical for both
artifacts, so there's no value in duplicating them).

sideEffects already uses 'dist/**/*.css' / 'dist/**/test-helpers.js' globs
that match both subdirectories — no change needed.

End-to-end verification (esbuild --minify, NODE_ENV='production'):

  - dist/production/UnderlineNav/UnderlineNav.js: 0 useDevOnlyEffect refs.
  - dist/production/internal/hooks/useDevOnlyEffect.js: not emitted
    (rollup tree-shook it because no caller references it).
  - Consumer prod bundle: 0 useDevOnlyEffect refs, 0 assertion message
    text. The closure body, deps array, and call site are all gone.
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 22, 2026

🦋 Changeset detected

Latest commit: 2f8618f

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 the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label May 22, 2026
@github-actions
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. 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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant