From 68c262d933265f2ac140f8f23a0d6824e46cd622 Mon Sep 17 00:00:00 2001 From: Emily Xiong Date: Wed, 3 May 2023 04:41:20 -0400 Subject: [PATCH] fix(react): fix external npm packages for rollup (#16713) --- .../packages/rollup/executors/rollup.json | 5 +- .../library/lib/add-rollup-build-target.ts | 2 +- .../src/generators/library/library.spec.ts | 10 ++-- .../src/executors/rollup/rollup.impl.spec.ts | 50 ++++++++++++++++++- .../src/executors/rollup/rollup.impl.ts | 23 ++++++--- .../rollup/src/executors/rollup/schema.d.ts | 2 +- .../rollup/src/executors/rollup/schema.json | 15 ++++-- 7 files changed, 87 insertions(+), 20 deletions(-) diff --git a/docs/generated/packages/rollup/executors/rollup.json b/docs/generated/packages/rollup/executors/rollup.json index f72a7dc07b..96cc44def2 100644 --- a/docs/generated/packages/rollup/executors/rollup.json +++ b/docs/generated/packages/rollup/executors/rollup.json @@ -66,7 +66,10 @@ "external": { "type": "array", "description": "A list of external modules that will not be bundled (`react`, `react-dom`, etc.).", - "items": { "type": "string" } + "oneOf": [ + { "type": "string", "enum": ["all", "none"] }, + { "type": "array", "items": { "type": "string" } } + ] }, "watch": { "type": "boolean", diff --git a/packages/react/src/generators/library/lib/add-rollup-build-target.ts b/packages/react/src/generators/library/lib/add-rollup-build-target.ts index 235d313772..e1dd26ac80 100644 --- a/packages/react/src/generators/library/lib/add-rollup-build-target.ts +++ b/packages/react/src/generators/library/lib/add-rollup-build-target.ts @@ -38,7 +38,7 @@ export async function addRollupBuildTarget( const { targets } = readProjectConfiguration(host, options.name); const { libsDir } = getWorkspaceLayout(host); - const external: string[] = []; + const external: string[] = ['react', 'react-dom']; if (options.style === '@emotion/styled') { external.push('@emotion/react/jsx-runtime'); diff --git a/packages/react/src/generators/library/library.spec.ts b/packages/react/src/generators/library/library.spec.ts index 9a96adecee..5ff1a1c2e1 100644 --- a/packages/react/src/generators/library/library.spec.ts +++ b/packages/react/src/generators/library/library.spec.ts @@ -505,7 +505,7 @@ describe('lib', () => { executor: '@nx/rollup:rollup', outputs: ['{options.outputPath}'], options: { - external: ['react/jsx-runtime'], + external: ['react', 'react-dom', 'react/jsx-runtime'], entryFile: 'libs/my-lib/src/index.ts', outputPath: 'dist/libs/my-lib', project: 'libs/my-lib/package.json', @@ -544,7 +544,7 @@ describe('lib', () => { expect(config.targets.build).toMatchObject({ options: { - external: ['react/jsx-runtime'], + external: ['react', 'react-dom', 'react/jsx-runtime'], }, }); expect(babelrc.plugins).toEqual([ @@ -566,7 +566,7 @@ describe('lib', () => { expect(config.targets.build).toMatchObject({ options: { - external: ['@emotion/react/jsx-runtime'], + external: ['react', 'react-dom', '@emotion/react/jsx-runtime'], }, }); expect(babelrc.plugins).toEqual(['@emotion/babel-plugin']); @@ -588,7 +588,7 @@ describe('lib', () => { expect(config.targets.build).toMatchObject({ options: { - external: ['react/jsx-runtime'], + external: ['react', 'react-dom', 'react/jsx-runtime'], }, }); expect(babelrc.plugins).toEqual(['styled-jsx/babel']); @@ -606,7 +606,7 @@ describe('lib', () => { expect(config.targets.build).toMatchObject({ options: { - external: ['react/jsx-runtime'], + external: ['react', 'react-dom', 'react/jsx-runtime'], }, }); }); diff --git a/packages/rollup/src/executors/rollup/rollup.impl.spec.ts b/packages/rollup/src/executors/rollup/rollup.impl.spec.ts index e04f60b1c0..168af2cbbf 100644 --- a/packages/rollup/src/executors/rollup/rollup.impl.spec.ts +++ b/packages/rollup/src/executors/rollup/rollup.impl.spec.ts @@ -150,9 +150,13 @@ describe('rollupExecutor', () => { }); }); - it(`should treat npm dependencies as external`, () => { + it(`should treat npm dependencies as external if external is all`, () => { const options = createRollupOptions( - normalizeRollupExecutorOptions(testOptions, '/root', '/root/src'), + normalizeRollupExecutorOptions( + { ...testOptions, external: 'all' }, + '/root', + '/root/src' + ), [], context, { name: 'example' }, @@ -166,5 +170,47 @@ describe('rollupExecutor', () => { expect(external('lodash/fp', '', false)).toBe(true); expect(external('rxjs', '', false)).toBe(false); }); + + it(`should not treat npm dependencies as external if external is none`, () => { + const options = createRollupOptions( + normalizeRollupExecutorOptions( + { ...testOptions, external: 'none' }, + '/root', + '/root/src' + ), + [], + context, + { name: 'example' }, + '/root/src', + ['lodash'] + ); + + const external = options[0].external as rollup.IsExternal; + + expect(external('lodash', '', false)).toBe(false); + expect(external('lodash/fp', '', false)).toBe(false); + expect(external('rxjs', '', false)).toBe(false); + }); + + it(`should set external based on options`, () => { + const options = createRollupOptions( + normalizeRollupExecutorOptions( + { ...testOptions, external: ['rxjs'] }, + '/root', + '/root/src' + ), + [], + context, + { name: 'example' }, + '/root/src', + ['lodash'] + ); + + const external = options[0].external as rollup.IsExternal; + + expect(external('lodash', '', false)).toBe(false); + expect(external('lodash/fp', '', false)).toBe(false); + expect(external('rxjs', '', false)).toBe(true); + }); }); }); diff --git a/packages/rollup/src/executors/rollup/rollup.impl.ts b/packages/rollup/src/executors/rollup/rollup.impl.ts index 9228266308..5b2a13ce6a 100644 --- a/packages/rollup/src/executors/rollup/rollup.impl.ts +++ b/packages/rollup/src/executors/rollup/rollup.impl.ts @@ -271,10 +271,18 @@ export function createRollupOptions( analyze(), ]; - const externalPackages = dependencies - .map((d) => d.name) - .concat(options.external || []) - .concat(Object.keys(packageJson.dependencies || {})); + let externalPackages = [ + ...Object.keys(packageJson.dependencies || {}), + ...Object.keys(packageJson.peerDependencies || {}), + ]; // If external is set to none, include all dependencies and peerDependencies in externalPackages + if (options.external === 'all') { + externalPackages = externalPackages + .concat(dependencies.map((d) => d.name)) + .concat(npmDeps); + } else if (Array.isArray(options.external) && options.external.length > 0) { + externalPackages = externalPackages.concat(options.external); + } + externalPackages = [...new Set(externalPackages)]; const rollupConfig = { input: options.outputFileName @@ -289,10 +297,11 @@ export function createRollupOptions( entryFileNames: `[name].${format === 'esm' ? 'js' : 'cjs'}`, chunkFileNames: `[name].${format === 'esm' ? 'js' : 'cjs'}`, }, - external: (id) => - externalPackages.some( + external: (id: string) => { + return externalPackages.some( (name) => id === name || id.startsWith(`${name}/`) - ) || npmDeps.some((name) => id === name || id.startsWith(`${name}/`)), // Could be a deep import + ); // Could be a deep import + }, plugins, }; diff --git a/packages/rollup/src/executors/rollup/schema.d.ts b/packages/rollup/src/executors/rollup/schema.d.ts index 45dcf6b930..a2bc5d5689 100644 --- a/packages/rollup/src/executors/rollup/schema.d.ts +++ b/packages/rollup/src/executors/rollup/schema.d.ts @@ -19,7 +19,7 @@ export interface RollupExecutorOptions { main: string; outputFileName?: string; extractCss?: boolean | string; - external?: string[]; + external?: string[] | 'all' | 'none'; rollupConfig?: string | string[]; watch?: boolean; assets?: any[]; diff --git a/packages/rollup/src/executors/rollup/schema.json b/packages/rollup/src/executors/rollup/schema.json index e728dca1e1..a9035a8def 100644 --- a/packages/rollup/src/executors/rollup/schema.json +++ b/packages/rollup/src/executors/rollup/schema.json @@ -66,9 +66,18 @@ "external": { "type": "array", "description": "A list of external modules that will not be bundled (`react`, `react-dom`, etc.).", - "items": { - "type": "string" - } + "oneOf": [ + { + "type": "string", + "enum": ["all", "none"] + }, + { + "type": "array", + "items": { + "type": "string" + } + } + ] }, "watch": { "type": "boolean",