use exports conditions for DEV checks (#2131)

This improves the handling of DEV-only warnings by producing a new `package.json#exports` entrypoint for `"development"`, with the actual code generated using SWC. All warning logs has been completely eliminated from the production build.
iTwin · Jul 5, 2024 · 9673bf6 · 9673bf6
1 parent 2bcf799
commit 9673bf6
Show file tree

Hide file tree

Showing 10 changed files with 108 additions and 42 deletions.
diff --git a/.changeset/nice-students-matter.md b/.changeset/nice-students-matter.md
@@ -0,0 +1,5 @@
+---
+'@itwin/itwinui-react': minor
+---
+
+DEV-only warnings will now only be properly excluded from the PROD bundle. This is done using a separate `"development"` entrypoint listed in `package.json#exports`.
diff --git a/packages/itwinui-react/.gitignore b/packages/itwinui-react/.gitignore
@@ -7,6 +7,8 @@ build
 # Compiled Components
 cjs
 esm
+DEV-cjs
+DEV-esm
 
 # Build outputs
 react-table.d.ts

diff --git a/packages/itwinui-react/package.json b/packages/itwinui-react/package.json
@@ -11,10 +11,12 @@
     ".": {
       "import": {
         "types": "./esm/index.d.ts",
+        "development": "./DEV-esm/index.js",
         "default": "./esm/index.js"
       },
       "require": {
         "types": "./cjs/index.d.ts",
+        "development": "./DEV-cjs/index.js",
         "default": "./cjs/index.js"
       }
     },
@@ -27,8 +29,16 @@
       }
     },
     "./styles.css": "./styles.css",
-    "./cjs": "./cjs/index.js",
-    "./esm": "./esm/index.js",
+    "./cjs": {
+      "types": "./cjs/index.d.ts",
+      "development": "./DEV-cjs/index.js",
+      "default": "./cjs/index.js"
+    },
+    "./esm": {
+      "types": "./esm/index.d.ts",
+      "development": "./DEV-esm/index.js",
+      "default": "./esm/index.js"
+    },
     "./package.json": "./package.json"
   },
   "typesVersions": {
@@ -41,6 +51,8 @@
   "files": [
     "cjs",
     "esm",
+    "DEV-esm",
+    "DEV-cjs",
     "styles.css",
     "CHANGELOG.md",
     "LICENSE.md"
@@ -72,7 +84,7 @@
     "build:types": "tsc -p tsconfig.build.json --outDir esm && tsc -p tsconfig.build.json --outDir cjs",
     "build:styles": "vite build src/styles.js",
     "build:post": "node ./scripts/postBuild.mjs",
-    "clean:build": "rimraf esm && rimraf cjs && rimraf react-table.d.ts",
+    "clean:build": "rimraf esm && rimraf cjs && rimraf DEV-esm && rimraf DEV-cjs",
     "clean:coverage": "rimraf coverage",
     "clean": "rimraf .turbo && pnpm clean:coverage && pnpm clean:build && rimraf node_modules",
     "test": "pnpm test:types && pnpm test:unit",

diff --git a/packages/itwinui-react/scripts/build.mjs b/packages/itwinui-react/scripts/build.mjs
@@ -26,6 +26,9 @@ const swcOptions = {
     'jsc.target=es2020',
     'jsc.minify.format.comments=false',
     'jsc.externalHelpers=true',
+
+    'jsc.minify.compress.defaults=false', // Disable default compress options
+    'jsc.minify.compress.dead_code=true', // Remove dead code (useful for removing NODE_ENV checks in production)
   ].join(' -C '),
 
   get esm() {
@@ -47,8 +50,24 @@ const swcOptions = {
   },
 };
 
-execSync(`pnpm swc src -d esm ${swcOptions.esm}`);
+// ----------------------------------------------------------------------------
+
+execSync(`pnpm swc src -d DEV-esm ${swcOptions.esm}`, {
+  env: { ...process.env, NODE_ENV: 'development' },
+});
+console.log('✓ Built esm (DEV).');
+
+execSync(`pnpm swc src -d DEV-cjs ${swcOptions.cjs}`, {
+  env: { ...process.env, NODE_ENV: 'development' },
+});
+console.log('✓ Built cjs (DEV).');
+
+execSync(`pnpm swc src -d esm ${swcOptions.esm}`, {
+  env: { ...process.env, NODE_ENV: 'production' },
+});
 console.log('✓ Built esm.');
 
-execSync(`pnpm swc src -d cjs ${swcOptions.cjs}`);
+execSync(`pnpm swc src -d cjs ${swcOptions.cjs}`, {
+  env: { ...process.env, NODE_ENV: 'production' },
+});
 console.log('✓ Built cjs.');
diff --git a/packages/itwinui-react/src/core/Breadcrumbs/Breadcrumbs.tsx b/packages/itwinui-react/src/core/Breadcrumbs/Breadcrumbs.tsx
@@ -15,7 +15,7 @@ import type { PolymorphicForwardRefComponent } from '../../utils/index.js';
 import { Button } from '../Buttons/Button.js';
 import { Anchor } from '../Typography/Anchor.js';
 
-const logWarningInDev = createWarningLogger();
+const logWarning = createWarningLogger();
 
 type BreadcrumbsProps = {
   /**
@@ -200,9 +200,11 @@ const ListItem = ({
     children?.type === 'a' ||
     children?.type === Button
   ) {
-    logWarningInDev(
-      'Directly using Button/a/span as Breadcrumbs children is deprecated, please use `Breadcrumbs.Item` instead.',
-    );
+    if (process.env.NODE_ENV === 'development') {
+      logWarning(
+        'Directly using Button/a/span as Breadcrumbs children is deprecated, please use `Breadcrumbs.Item` instead.',
+      );
+    }
     children = <BreadcrumbsItem {...children.props} />;
   }
 

diff --git a/packages/itwinui-react/src/core/Menu/MenuItem.tsx b/packages/itwinui-react/src/core/Menu/MenuItem.tsx
@@ -15,7 +15,7 @@ import { ListItem } from '../List/ListItem.js';
 import type { ListItemOwnProps } from '../List/ListItem.js';
 import cx from 'classnames';
 
-const logWarningInDev = createWarningLogger();
+const logWarning = createWarningLogger();
 
 export type MenuItemProps = {
   /**
@@ -100,8 +100,12 @@ export const MenuItem = React.forwardRef((props, forwardedRef) => {
     ...rest
   } = props;
 
-  if (onClickProp != null && subMenuItems.length > 0) {
-    logWarningInDev(
+  if (
+    process.env.NODE_ENV === 'development' &&
+    onClickProp != null &&
+    subMenuItems.length > 0
+  ) {
+    logWarning(
       'Passing a non-empty submenuItems array and onClick to MenuItem at the same time is not supported. This is because when a non empty submenuItems array is passed, clicking the MenuItem toggles the submenu visibility.',
     );
   }

diff --git a/packages/itwinui-react/src/core/Table/Table.tsx b/packages/itwinui-react/src/core/Table/Table.tsx
@@ -83,7 +83,7 @@ const COLUMN_MIN_WIDTHS = {
   withExpander: 108, // expander column should be wider to accommodate the expander icon
 };
 
-const logWarningInDev = createWarningLogger();
+const logWarning = createWarningLogger();
 
 export type TablePaginatorRendererProps = {
   /**
@@ -635,9 +635,11 @@ export const Table = <
 
   if (columns.length === 1 && 'columns' in columns[0]) {
     headerGroups = _headerGroups.slice(1);
-    logWarningInDev(
-      `Table's \`columns\` prop should not have a top-level \`Header\` or sub-columns. They are only allowed to be passed for backwards compatibility.\n See https://github.com/iTwin/iTwinUI/wiki/iTwinUI-react-v2-migration-guide#breaking-changes`,
-    );
+    if (process.env.NODE_ENV === 'development') {
+      logWarning(
+        `Table's \`columns\` prop should not have a top-level \`Header\` or sub-columns. They are only allowed to be passed for backwards compatibility.\n See https://github.com/iTwin/iTwinUI/wiki/iTwinUI-react-v2-migration-guide#breaking-changes`,
+      );
+    }
   }
 
   const ariaDataAttributes = Object.entries(rest).reduce(

diff --git a/packages/itwinui-react/src/styles.js/vite.config.mjs b/packages/itwinui-react/src/styles.js/vite.config.mjs
@@ -95,6 +95,8 @@ const distEsmDir = path.join(distDir, 'esm');
 const distCjsDir = path.join(distDir, 'cjs');
 const outCjsDir = path.join(root, 'cjs');
 const outEsmDir = path.join(root, 'esm');
+const outEsmDevDir = path.join(root, 'DEV-esm');
+const outCjsDevDir = path.join(root, 'DEV-cjs');
 
 const copyBuildOutput = async () => {
   // create cjs/ and esm/ directories if they don't exist
@@ -105,15 +107,23 @@ const copyBuildOutput = async () => {
     await fs.promises.mkdir(outEsmDir);
   }
 
-  // copy styles.js from src/styles.js/dist/ into cjs/ and esm/
+  // copy styles.js from src/styles.js/dist/ into cjs/, esm/, DEV-cjs/, and DEV-esm/
   await fs.promises.copyFile(
     path.join(distEsmDir, 'styles.js'),
     path.join(outEsmDir, 'styles.js'),
   );
+  await fs.promises.copyFile(
+    path.join(distEsmDir, 'styles.js'),
+    path.join(outEsmDevDir, 'styles.js'),
+  );
   await fs.promises.copyFile(
     path.join(distCjsDir, 'styles.js'),
     path.join(outCjsDir, 'styles.js'),
   );
+  await fs.promises.copyFile(
+    path.join(distCjsDir, 'styles.js'),
+    path.join(outCjsDevDir, 'styles.js'),
+  );
 
   // copy styles.css from src/styles.js/dist/ into <root>/
   await fs.promises.copyFile(

diff --git a/packages/itwinui-react/src/utils/functions/dev.ts b/packages/itwinui-react/src/utils/functions/dev.ts
@@ -14,30 +14,31 @@ const isVitest = typeof (globalThis as any).__vitest_index__ !== 'undefined';
 
 const isUnitTest = isJest || isVitest || isMocha;
 
-let isDev = false;
-
-// wrapping in try-catch because process might be undefined
-try {
-  isDev = process.env.NODE_ENV !== 'production' && !isUnitTest;
-} catch {}
-
 /**
- * Logs message one time only in dev environments.
+ * Returns a function that can be used to log one-time warnings in dev environments.
+ *
+ * **Note**: The actual log call should be wrapped in a check against `process.env.NODE_ENV === 'development'`
+ * to ensure that it is removed from the production build output (by SWC).
+ * Read more about the [`NODE_ENV` convention](https://nodejs.org/en/learn/getting-started/nodejs-the-difference-between-development-and-production).
  *
  * @example
- * const logWarningInDev = createWarningLogger();
- * logWarningInDev("please don't use this")
+ * const logWarning = createWarningLogger();
+ *
+ * if (process.env.NODE_ENV === 'development') {
+ *   logWarning("please don't use this")
+ * }
  */
-const createWarningLogger = !isDev
-  ? () => () => {}
-  : () => {
-      let logged = false;
-      return (message: string) => {
-        if (!logged) {
-          console.warn(message);
-          logged = true;
-        }
-      };
-    };
+const createWarningLogger =
+  process.env.NODE_ENV === 'development' && !isUnitTest
+    ? () => {
+        let logged = false;
+        return (message: string) => {
+          if (!logged) {
+            console.warn(message);
+            logged = true;
+          }
+        };
+      }
+    : () => () => {};
 
-export { isUnitTest, isDev, createWarningLogger };
+export { isUnitTest, createWarningLogger };
diff --git a/packages/itwinui-react/src/utils/hooks/useGlobals.ts b/packages/itwinui-react/src/utils/hooks/useGlobals.ts
@@ -4,7 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 import * as React from 'react';
 import { ThemeContext } from '../../core/ThemeProvider/ThemeContext.js';
-import { isDev } from '../functions/dev.js';
+import { isUnitTest } from '../functions/dev.js';
 
 const didLogWarning = {
   fontSize: false,
@@ -31,7 +31,12 @@ export const useThemeProviderWarning = (
   themeContext: React.ContextType<typeof ThemeContext>,
 ) => {
   React.useEffect(() => {
-    if (isDev && !didLogWarning.themeProvider && !themeContext) {
+    if (
+      process.env.NODE_ENV === 'development' &&
+      !isUnitTest &&
+      !didLogWarning.themeProvider &&
+      !themeContext
+    ) {
       console.error(
         'iTwinUI components must be used within a tree wrapped in a ThemeProvider.',
       );
@@ -45,7 +50,11 @@ export const useThemeProviderWarning = (
 /** Shows console error if the page changes the root font size */
 const useRootFontSizeWarning = () => {
   React.useEffect(() => {
-    if (isDev && !didLogWarning.fontSize) {
+    if (
+      process.env.NODE_ENV === 'development' &&
+      !isUnitTest &&
+      !didLogWarning.fontSize
+    ) {
       const rootFontSize = parseInt(
         getComputedStyle(document.documentElement).fontSize,
       );