diff --git a/.changeset/dev-only-effect-dual-build.md b/.changeset/dev-only-effect-dual-build.md new file mode 100644 index 00000000000..9946925a2ad --- /dev/null +++ b/.changeset/dev-only-effect-dual-build.md @@ -0,0 +1,14 @@ +--- +'@primer/react': patch +--- + +Dev-only effects (the `if (__DEV__) { useEffect(...) }` pattern with an +`eslint-disable react-hooks/rules-of-hooks` comment) are now expressed via a +new internal `useDevOnlyEffect` hook, and the published package ships dual +`dist/development/` and `dist/production/` artifacts. Consumers' bundlers +automatically pick the right one via `package.json` `exports` `development` / +`production` import conditions, and the production artifact has every +`useDevOnlyEffect(...)` call statement — including the effect-callback closure +and the deps array — removed at publish time by a small babel plugin. The +public API is unchanged; this is purely a runtime/byte-size win in production +builds. diff --git a/packages/react/package.json b/packages/react/package.json index 19b28d8e2c2..3ff18e48f6b 100644 --- a/packages/react/package.json +++ b/packages/react/package.json @@ -3,28 +3,38 @@ "type": "module", "version": "38.24.0", "description": "An implementation of GitHub's Primer Design System using React", - "main": "./dist/index.js", - "module": "./dist/index.js", + "main": "./dist/production/index.js", + "module": "./dist/production/index.js", "exports": { ".": { "types": "./dist/index.d.ts", - "default": "./dist/index.js" + "development": "./dist/development/index.js", + "production": "./dist/production/index.js", + "default": "./dist/production/index.js" }, "./experimental": { "types": "./dist/experimental/index.d.ts", - "default": "./dist/experimental/index.js" + "development": "./dist/development/experimental/index.js", + "production": "./dist/production/experimental/index.js", + "default": "./dist/production/experimental/index.js" }, "./deprecated": { "types": "./dist/deprecated/index.d.ts", - "default": "./dist/deprecated/index.js" + "development": "./dist/development/deprecated/index.js", + "production": "./dist/production/deprecated/index.js", + "default": "./dist/production/deprecated/index.js" }, "./next": { "types": "./dist/next/index.d.ts", - "default": "./dist/next/index.js" + "development": "./dist/development/next/index.js", + "production": "./dist/production/next/index.js", + "default": "./dist/production/next/index.js" }, "./test-helpers": { "types": "./dist/utils/test-helpers.d.ts", - "default": "./dist/test-helpers.js" + "development": "./dist/development/test-helpers.js", + "production": "./dist/production/test-helpers.js", + "default": "./dist/production/test-helpers.js" }, "./generated/components.json": "./generated/components.json", "./generated/hooks.json": "./generated/hooks.json" diff --git a/packages/react/rollup.config.mjs b/packages/react/rollup.config.mjs index 81cf54a2938..a8c4cb2ba9b 100644 --- a/packages/react/rollup.config.mjs +++ b/packages/react/rollup.config.mjs @@ -47,180 +47,207 @@ const postcssModulesOptions = { generateScopedName: 'prc-[folder]-[local]-[hash:base64:5]', } -const baseConfig = { - input: { - ...getEntrypointsFromInput(input), - // "./test-helpers" - 'test-helpers': 'src/utils/test-helpers.tsx', - }, - plugins: [ - babel({ - extensions, - exclude: /node_modules/, - babelHelpers: 'inline', - babelrc: false, - configFile: false, - presets: [ - '@babel/preset-typescript', - [ - '@babel/preset-react', - { - modules: false, - runtime: 'automatic', - }, - ], - ], - plugins: [ - [ - 'babel-plugin-react-compiler', - { - target: '18', - sources: filepath => isSupported(filepath), - }, +const baseInput = { + ...getEntrypointsFromInput(input), + // "./test-helpers" + 'test-helpers': 'src/utils/test-helpers.tsx', +} + +const stripDevOnlyEffectPluginPath = new URL('./script/babel-plugin-strip-dev-only-effect.cjs', import.meta.url) + .pathname + +/** + * Build configuration factory. Produces one rollup config per mode. + * + * - mode='development': __DEV__ is replaced with `true`. useDevOnlyEffect + * calls run as written (its internal `if (__DEV__)` guard collapses to + * the live `useEffect`). All `if (__DEV__) { ... }` blocks elsewhere in + * the codebase run their dev branch. + * - mode='production': __DEV__ is replaced with `false`. Every + * `useDevOnlyEffect(...)` call statement is removed by the strip plugin + * (the call, the effect closure, and the deps array — all gone). Other + * `if (__DEV__) { ... }` blocks compile to `if (false) { ... }` and are + * eliminated by the consumer's bundler DCE. + * + * The two outputs live under `dist/development/` and `dist/production/`; + * package.json's `exports` field selects between them via the + * `development` / `production` import conditions. + */ +function makeConfig(mode) { + const isProduction = mode === 'production' + return { + input: baseInput, + external: dependencies.map(createPackageRegex), + output: { + interop: 'auto', + dir: `dist/${mode}`, + format: 'esm', + preserveModules: true, + preserveModulesRoot: 'src', + }, + plugins: [ + babel({ + extensions, + exclude: /node_modules/, + babelHelpers: 'inline', + babelrc: false, + configFile: false, + presets: [ + '@babel/preset-typescript', + [ + '@babel/preset-react', + { + modules: false, + runtime: 'automatic', + }, + ], ], - 'macros', - 'add-react-displayname', - 'dev-expression', - 'babel-plugin-styled-components', - '@babel/plugin-proposal-nullish-coalescing-operator', - '@babel/plugin-proposal-optional-chaining', - [ - 'babel-plugin-transform-replace-expressions', - { - replace: { - __DEV__: "process.env.NODE_ENV !== 'production'", + plugins: [ + [ + 'babel-plugin-react-compiler', + { + target: '18', + sources: filepath => isSupported(filepath), }, - }, + ], + 'macros', + 'add-react-displayname', + // In production, drop every useDevOnlyEffect(...) call statement + // (and its effect closure and deps array) before __DEV__ is replaced. + // Both the strip plugin and the replace below must run BEFORE + // dev-expression — that plugin rewrites `__DEV__` to + // `process.env.NODE_ENV !== "production"`, which would shadow our + // baked-in literal and leave a runtime check in the artifact. + ...(isProduction ? [stripDevOnlyEffectPluginPath] : []), + [ + 'babel-plugin-transform-replace-expressions', + { + replace: { + __DEV__: isProduction ? 'false' : 'true', + }, + }, + ], + 'dev-expression', + 'babel-plugin-styled-components', + '@babel/plugin-proposal-nullish-coalescing-operator', + '@babel/plugin-proposal-optional-chaining', ], - ], - }), - resolve({ - extensions, - }), - commonjs({ - extensions, - }), - importCSS({ - modulesRoot: 'src', - postcssPlugins: [postcssPresetPrimer()], - postcssModulesOptions, - }), - - /** - * This custom rollup plugin allows us to preserve directives in source - * code, such as "use client", in order to support React Server Components. - * - * The source for this plugin is inspired by: - * https://github.com/Ephem/rollup-plugin-preserve-directives - */ - { - name: 'preserve-directives', - transform(code) { - const ast = this.parse(code) - if (ast.type !== 'Program' || !ast.body) { - return { - code, - ast, - map: null, + }), + resolve({ + extensions, + }), + commonjs({ + extensions, + }), + importCSS({ + modulesRoot: 'src', + postcssPlugins: [postcssPresetPrimer()], + postcssModulesOptions, + }), + + /** + * This custom rollup plugin allows us to preserve directives in source + * code, such as "use client", in order to support React Server Components. + * + * The source for this plugin is inspired by: + * https://github.com/Ephem/rollup-plugin-preserve-directives + */ + { + name: 'preserve-directives', + transform(code) { + const ast = this.parse(code) + if (ast.type !== 'Program' || !ast.body) { + return { + code, + ast, + map: null, + } } - } - let hasClientDirective = false + let hasClientDirective = false - for (const node of ast.body) { - if (!node) { - continue - } + for (const node of ast.body) { + if (!node) { + continue + } + + if (node.type !== 'ExpressionStatement') { + continue + } - if (node.type !== 'ExpressionStatement') { - continue + if (node.directive === 'use client') { + hasClientDirective = true + break + } } - if (node.directive === 'use client') { - hasClientDirective = true - break + if (hasClientDirective) { + return { + code, + ast, + map: null, + meta: { + hasClientDirective: true, + }, + } } - } - if (hasClientDirective) { return { code, ast, map: null, - meta: { - hasClientDirective: true, - }, - } - } - - return { - code, - ast, - map: null, - } - }, - renderChunk: { - order: 'post', - handler(code, chunk, options) { - // If `preserveModules` is not set to true, we can't be sure if the client - // directive corresponds to the whole chunk or just a part of it. - if (!options.preserveModules) { - return undefined } + }, + renderChunk: { + order: 'post', + handler(code, chunk, options) { + // If `preserveModules` is not set to true, we can't be sure if the client + // directive corresponds to the whole chunk or just a part of it. + if (!options.preserveModules) { + return undefined + } - let chunkHasClientDirective = false + let chunkHasClientDirective = false - for (const moduleId of Object.keys(chunk.modules)) { - const hasClientDirective = this.getModuleInfo(moduleId)?.meta?.hasClientDirective - if (hasClientDirective) { - chunkHasClientDirective = true - break + for (const moduleId of Object.keys(chunk.modules)) { + const hasClientDirective = this.getModuleInfo(moduleId)?.meta?.hasClientDirective + if (hasClientDirective) { + chunkHasClientDirective = true + break + } } - } - if (chunkHasClientDirective) { - const transformed = new MagicString(code) - transformed.prepend(`"use client";\n`) - const sourcemap = transformed.generateMap({ - includeContent: true, - }) - return { - code: transformed.toString(), - map: sourcemap, + if (chunkHasClientDirective) { + const transformed = new MagicString(code) + transformed.prepend(`"use client";\n`) + const sourcemap = transformed.generateMap({ + includeContent: true, + }) + return { + code: transformed.toString(), + map: sourcemap, + } } - } - return null + return null + }, }, }, + ], + onwarn(warning, defaultHandler) { + // Dependencies or modules may use "use client" as an indicator for React + // Server Components that this module should only be loaded on the client. + if (warning.code === 'MODULE_LEVEL_DIRECTIVE' && warning.message.includes('use client')) { + return + } + + if (warning.code === 'CIRCULAR_DEPENDENCY') { + throw warning + } + + defaultHandler(warning) }, - ], - onwarn(warning, defaultHandler) { - // Dependencies or modules may use "use client" as an indicator for React - // Server Components that this module should only be loaded on the client. - if (warning.code === 'MODULE_LEVEL_DIRECTIVE' && warning.message.includes('use client')) { - return - } - - if (warning.code === 'CIRCULAR_DEPENDENCY') { - throw warning - } - - defaultHandler(warning) - }, + } } -export default [ - // ESM - { - ...baseConfig, - external: dependencies.map(createPackageRegex), - output: { - interop: 'auto', - dir: 'dist', - format: 'esm', - preserveModules: true, - preserveModulesRoot: 'src', - }, - }, -] +export default [makeConfig('development'), makeConfig('production')] diff --git a/packages/react/script/babel-plugin-strip-dev-only-effect.cjs b/packages/react/script/babel-plugin-strip-dev-only-effect.cjs new file mode 100644 index 00000000000..8818f1ebc0c --- /dev/null +++ b/packages/react/script/babel-plugin-strip-dev-only-effect.cjs @@ -0,0 +1,58 @@ +'use strict' + +/** + * Removes every `useDevOnlyEffect(...)` call statement whose identifier + * resolves to an import from a `useDevOnlyEffect` module. Used by the + * production rollup build to eliminate dev-only effect call sites entirely + * — the call, the effect-callback closure, and the deps array — from the + * `dist/production/` artifact. + * + * The development rollup build does NOT apply this plugin: it sets + * `__DEV__: true` so the hook's internal `if (__DEV__)` guard collapses to + * `useEffect(effect, deps)` after constant folding, and the call site runs + * as expected. + * + * Selection rules (kept conservative on purpose): + * + * - Only matches direct identifier calls: `useDevOnlyEffect(...)`. + * Aliased imports (`import {useDevOnlyEffect as foo}`) are left alone; + * callers should not alias internal hooks anyway. + * + * - Only matches when the local identifier's binding resolves to an + * ES module import whose source path contains the substring + * `useDevOnlyEffect`. This avoids matching same-named locals or + * re-exports from unrelated modules. + * + * - Only removes statement-level calls + * (`useDevOnlyEffect(fn, deps);` as an `ExpressionStatement`). + * A hook is invalid in expression positions, so this captures every + * legal usage. + * + * After the plugin runs the import binding may become unused, in which case + * the unused-import handling in @babel/preset-react or rollup's tree-shaking + * drops it; the hook module itself is then dropped from the bundle because + * `package.json` declares non-CSS files have no side effects. + */ + +const HOOK_NAME = 'useDevOnlyEffect' + +module.exports = function stripUseDevOnlyEffect({types: t}) { + return { + name: 'strip-use-dev-only-effect', + visitor: { + ExpressionStatement(path) { + const expr = path.node.expression + if (!t.isCallExpression(expr)) return + if (!t.isIdentifier(expr.callee, {name: HOOK_NAME})) return + + const binding = path.scope.getBinding(HOOK_NAME) + if (!binding || binding.kind !== 'module') return + const importDecl = binding.path.findParent(p => p.isImportDeclaration()) + if (!importDecl) return + if (!importDecl.node.source.value.includes('useDevOnlyEffect')) return + + path.remove() + }, + }, + } +} 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