From 48d198ae82412a290cd69ca45886e93279465107 Mon Sep 17 00:00:00 2001 From: ivan Date: Sun, 9 Apr 2023 21:54:42 +0800 Subject: [PATCH 1/4] fix(importAnalysis): warning on ExportAllDeclaration --- .../vite/src/node/plugins/importAnalysis.ts | 37 +++++++++++++++++-- .../src/node/plugins/importAnalysisBuild.ts | 5 ++- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/packages/vite/src/node/plugins/importAnalysis.ts b/packages/vite/src/node/plugins/importAnalysis.ts index 0a93e270213d99..613d72cf1244ed 100644 --- a/packages/vite/src/node/plugins/importAnalysis.ts +++ b/packages/vite/src/node/plugins/importAnalysis.ts @@ -585,7 +585,10 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { } } else if (needsInterop) { debug?.(`${url} needs interop`) - interopNamedImports(str(), importSpecifier, url, index) + interopNamedImports(str(), importSpecifier, url, index, { + config, + exportAll: false, + }) rewriteDone = true } } @@ -596,7 +599,10 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { url.includes(browserExternalId) && source.slice(expStart, start).includes('{') ) { - interopNamedImports(str(), importSpecifier, url, index) + interopNamedImports(str(), importSpecifier, url, index, { + config, + exportAll: false, + }) rewriteDone = true } if (!rewriteDone) { @@ -811,6 +817,10 @@ export function interopNamedImports( importSpecifier: ImportSpecifier, rewrittenUrl: string, importIndex: number, + options: { + config: ResolvedConfig + exportAll: boolean + }, ): void { const source = str.original const { @@ -831,7 +841,13 @@ export function interopNamedImports( } else { const exp = source.slice(expStart, expEnd) const rawUrl = source.slice(start, end) - const rewritten = transformCjsImport(exp, rewrittenUrl, rawUrl, importIndex) + const rewritten = transformCjsImport( + exp, + rewrittenUrl, + rawUrl, + importIndex, + options, + ) if (rewritten) { str.overwrite(expStart, expEnd, rewritten, { contentOnly: true }) } else { @@ -861,6 +877,10 @@ export function transformCjsImport( url: string, rawUrl: string, importIndex: number, + options: { + config: ResolvedConfig + exportAll: boolean + }, ): string | undefined { const node = ( parseJS(importExp, { @@ -869,7 +889,18 @@ export function transformCjsImport( }) as any ).body[0] as Node + // `export * from '...'` may cause unexpected problem, so give it a warning if ( + !options.exportAll && + node.type === 'ExportAllDeclaration' && + !node.exported + ) { + options.config.logger.warn( + colors.yellow( + `\n\`${importExp}\` may lose module exports after pre-bundling origin cjs module "${rawUrl}", please use named exports(e.g \`export {A,B} from "${rawUrl}"\`) instead.`, + ), + ) + } else if ( node.type === 'ImportDeclaration' || node.type === 'ExportNamedDeclaration' ) { diff --git a/packages/vite/src/node/plugins/importAnalysisBuild.ts b/packages/vite/src/node/plugins/importAnalysisBuild.ts index 8913ff5970dbc9..2b9b61daf26dc7 100644 --- a/packages/vite/src/node/plugins/importAnalysisBuild.ts +++ b/packages/vite/src/node/plugins/importAnalysisBuild.ts @@ -338,7 +338,10 @@ export function buildImportAnalysisPlugin(config: ResolvedConfig): Plugin { } } else if (needsInterop) { // config.logger.info(`${url} needs interop`) - interopNamedImports(str(), imports[index], url, index) + interopNamedImports(str(), imports[index], url, index, { + config, + exportAll: true, + }) rewriteDone = true } if (!rewriteDone) { From 360983d4ad8dd7b23dcd43d904c4f2944b07aa18 Mon Sep 17 00:00:00 2001 From: ivan Date: Sun, 9 Apr 2023 22:31:30 +0800 Subject: [PATCH 2/4] test: export * --- .../src/node/__tests__/plugins/import.spec.ts | 54 ++++++++++++++++--- .../vite/src/node/plugins/importAnalysis.ts | 31 +++++------ .../src/node/plugins/importAnalysisBuild.ts | 5 +- 3 files changed, 61 insertions(+), 29 deletions(-) diff --git a/packages/vite/src/node/__tests__/plugins/import.spec.ts b/packages/vite/src/node/__tests__/plugins/import.spec.ts index 792c58515f542e..d9ab51e09b1b71 100644 --- a/packages/vite/src/node/__tests__/plugins/import.spec.ts +++ b/packages/vite/src/node/__tests__/plugins/import.spec.ts @@ -1,9 +1,19 @@ -import { describe, expect, test } from 'vitest' +import { beforeEach, describe, expect, test, vi } from 'vitest' import { transformCjsImport } from '../../plugins/importAnalysis' describe('transformCjsImport', () => { const url = './node_modules/.vite/deps/react.js' const rawUrl = 'react' + const config: any = { + command: 'serve', + logger: { + warn: vi.fn(), + }, + } + + beforeEach(() => { + config.logger.warn.mockClear() + }) test('import specifier', () => { expect( @@ -12,6 +22,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + config, ), ).toBe( 'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' + @@ -22,7 +33,7 @@ describe('transformCjsImport', () => { test('import default specifier', () => { expect( - transformCjsImport('import React from "react"', url, rawUrl, 0), + transformCjsImport('import React from "react"', url, rawUrl, 0, config), ).toBe( 'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' + 'const React = __vite__cjsImport0_react.__esModule ? __vite__cjsImport0_react.default : __vite__cjsImport0_react', @@ -34,6 +45,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + config, ), ).toBe( 'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' + @@ -43,7 +55,13 @@ describe('transformCjsImport', () => { test('import all specifier', () => { expect( - transformCjsImport('import * as react from "react"', url, rawUrl, 0), + transformCjsImport( + 'import * as react from "react"', + url, + rawUrl, + 0, + config, + ), ).toBe( 'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' + 'const react = __vite__cjsImport0_react', @@ -51,13 +69,25 @@ describe('transformCjsImport', () => { }) test('export all specifier', () => { - expect(transformCjsImport('export * from "react"', url, rawUrl, 0)).toBe( - undefined, + expect( + transformCjsImport('export * from "react"', url, rawUrl, 0, config), + ).toBe(undefined) + + expect(config.logger.warn).toBeCalledWith( + expect.stringContaining(`export * from "react"`), ) expect( - transformCjsImport('export * as react from "react"', url, rawUrl, 0), + transformCjsImport( + 'export * as react from "react"', + url, + rawUrl, + 0, + config, + ), ).toBe(undefined) + + expect(config.logger.warn).toBeCalledTimes(1) }) test('export name specifier', () => { @@ -67,6 +97,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + config, ), ).toBe( 'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' + @@ -81,6 +112,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + config, ), ).toBe( 'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' + @@ -92,7 +124,13 @@ describe('transformCjsImport', () => { test('export default specifier', () => { expect( - transformCjsImport('export { default } from "react"', url, rawUrl, 0), + transformCjsImport( + 'export { default } from "react"', + url, + rawUrl, + 0, + config, + ), ).toBe( 'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' + 'const __vite__cjsExportDefault_0 = __vite__cjsImport0_react.__esModule ? __vite__cjsImport0_react.default : __vite__cjsImport0_react; ' + @@ -105,6 +143,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + config, ), ).toBe( 'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' + @@ -118,6 +157,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + config, ), ).toBe( 'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' + diff --git a/packages/vite/src/node/plugins/importAnalysis.ts b/packages/vite/src/node/plugins/importAnalysis.ts index 613d72cf1244ed..3b9f5f2a254278 100644 --- a/packages/vite/src/node/plugins/importAnalysis.ts +++ b/packages/vite/src/node/plugins/importAnalysis.ts @@ -585,10 +585,13 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { } } else if (needsInterop) { debug?.(`${url} needs interop`) - interopNamedImports(str(), importSpecifier, url, index, { + interopNamedImports( + str(), + importSpecifier, + url, + index, config, - exportAll: false, - }) + ) rewriteDone = true } } @@ -599,10 +602,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { url.includes(browserExternalId) && source.slice(expStart, start).includes('{') ) { - interopNamedImports(str(), importSpecifier, url, index, { - config, - exportAll: false, - }) + interopNamedImports(str(), importSpecifier, url, index, config) rewriteDone = true } if (!rewriteDone) { @@ -817,10 +817,8 @@ export function interopNamedImports( importSpecifier: ImportSpecifier, rewrittenUrl: string, importIndex: number, - options: { - config: ResolvedConfig - exportAll: boolean - }, + + config: ResolvedConfig, ): void { const source = str.original const { @@ -846,7 +844,7 @@ export function interopNamedImports( rewrittenUrl, rawUrl, importIndex, - options, + config, ) if (rewritten) { str.overwrite(expStart, expEnd, rewritten, { contentOnly: true }) @@ -877,10 +875,7 @@ export function transformCjsImport( url: string, rawUrl: string, importIndex: number, - options: { - config: ResolvedConfig - exportAll: boolean - }, + config: ResolvedConfig, ): string | undefined { const node = ( parseJS(importExp, { @@ -891,11 +886,11 @@ export function transformCjsImport( // `export * from '...'` may cause unexpected problem, so give it a warning if ( - !options.exportAll && + config.command === 'serve' && node.type === 'ExportAllDeclaration' && !node.exported ) { - options.config.logger.warn( + config.logger.warn( colors.yellow( `\n\`${importExp}\` may lose module exports after pre-bundling origin cjs module "${rawUrl}", please use named exports(e.g \`export {A,B} from "${rawUrl}"\`) instead.`, ), diff --git a/packages/vite/src/node/plugins/importAnalysisBuild.ts b/packages/vite/src/node/plugins/importAnalysisBuild.ts index 2b9b61daf26dc7..35c638ad07f787 100644 --- a/packages/vite/src/node/plugins/importAnalysisBuild.ts +++ b/packages/vite/src/node/plugins/importAnalysisBuild.ts @@ -338,10 +338,7 @@ export function buildImportAnalysisPlugin(config: ResolvedConfig): Plugin { } } else if (needsInterop) { // config.logger.info(`${url} needs interop`) - interopNamedImports(str(), imports[index], url, index, { - config, - exportAll: true, - }) + interopNamedImports(str(), imports[index], url, index, config) rewriteDone = true } if (!rewriteDone) { From a2a72da1123b19b2e4337963fb318ccf7d722bf5 Mon Sep 17 00:00:00 2001 From: ivan Date: Thu, 13 Apr 2023 12:11:13 +0800 Subject: [PATCH 3/4] chore: rewrite warning message --- packages/vite/src/node/plugins/importAnalysis.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/vite/src/node/plugins/importAnalysis.ts b/packages/vite/src/node/plugins/importAnalysis.ts index 3b9f5f2a254278..1c52e0edab5924 100644 --- a/packages/vite/src/node/plugins/importAnalysis.ts +++ b/packages/vite/src/node/plugins/importAnalysis.ts @@ -892,7 +892,7 @@ export function transformCjsImport( ) { config.logger.warn( colors.yellow( - `\n\`${importExp}\` may lose module exports after pre-bundling origin cjs module "${rawUrl}", please use named exports(e.g \`export {A,B} from "${rawUrl}"\`) instead.`, + `\nUnable to interop \`${importExp}\` and may lose module exports. Please export "${rawUrl}" as ESM or use named exports instead, e.g. \`export { A, B } from "${rawUrl}"\``, ), ) } else if ( From 7aeee409729ee399cd7443a5a74caee27d885783 Mon Sep 17 00:00:00 2001 From: ivan Date: Thu, 13 Apr 2023 13:40:44 +0800 Subject: [PATCH 4/4] feat: attach importer info --- .../src/node/__tests__/plugins/import.spec.ts | 29 +++++++++++++++++-- .../vite/src/node/plugins/importAnalysis.ts | 16 ++++++++-- .../src/node/plugins/importAnalysisBuild.ts | 9 +++++- 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/packages/vite/src/node/__tests__/plugins/import.spec.ts b/packages/vite/src/node/__tests__/plugins/import.spec.ts index d9ab51e09b1b71..679d5bdb3d0a38 100644 --- a/packages/vite/src/node/__tests__/plugins/import.spec.ts +++ b/packages/vite/src/node/__tests__/plugins/import.spec.ts @@ -22,6 +22,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + '', config, ), ).toBe( @@ -33,7 +34,14 @@ describe('transformCjsImport', () => { test('import default specifier', () => { expect( - transformCjsImport('import React from "react"', url, rawUrl, 0, config), + transformCjsImport( + 'import React from "react"', + url, + rawUrl, + 0, + '', + config, + ), ).toBe( 'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' + 'const React = __vite__cjsImport0_react.__esModule ? __vite__cjsImport0_react.default : __vite__cjsImport0_react', @@ -45,6 +53,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + '', config, ), ).toBe( @@ -60,6 +69,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + '', config, ), ).toBe( @@ -70,11 +80,18 @@ describe('transformCjsImport', () => { test('export all specifier', () => { expect( - transformCjsImport('export * from "react"', url, rawUrl, 0, config), + transformCjsImport( + 'export * from "react"', + url, + rawUrl, + 0, + 'modA', + config, + ), ).toBe(undefined) expect(config.logger.warn).toBeCalledWith( - expect.stringContaining(`export * from "react"`), + expect.stringContaining(`export * from "react"\` in modA`), ) expect( @@ -83,6 +100,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + '', config, ), ).toBe(undefined) @@ -97,6 +115,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + '', config, ), ).toBe( @@ -112,6 +131,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + '', config, ), ).toBe( @@ -129,6 +149,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + '', config, ), ).toBe( @@ -143,6 +164,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + '', config, ), ).toBe( @@ -157,6 +179,7 @@ describe('transformCjsImport', () => { url, rawUrl, 0, + '', config, ), ).toBe( diff --git a/packages/vite/src/node/plugins/importAnalysis.ts b/packages/vite/src/node/plugins/importAnalysis.ts index 1c52e0edab5924..119f0192cabb43 100644 --- a/packages/vite/src/node/plugins/importAnalysis.ts +++ b/packages/vite/src/node/plugins/importAnalysis.ts @@ -590,6 +590,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { importSpecifier, url, index, + importer, config, ) rewriteDone = true @@ -602,7 +603,14 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { url.includes(browserExternalId) && source.slice(expStart, start).includes('{') ) { - interopNamedImports(str(), importSpecifier, url, index, config) + interopNamedImports( + str(), + importSpecifier, + url, + index, + importer, + config, + ) rewriteDone = true } if (!rewriteDone) { @@ -817,7 +825,7 @@ export function interopNamedImports( importSpecifier: ImportSpecifier, rewrittenUrl: string, importIndex: number, - + importer: string, config: ResolvedConfig, ): void { const source = str.original @@ -844,6 +852,7 @@ export function interopNamedImports( rewrittenUrl, rawUrl, importIndex, + importer, config, ) if (rewritten) { @@ -875,6 +884,7 @@ export function transformCjsImport( url: string, rawUrl: string, importIndex: number, + importer: string, config: ResolvedConfig, ): string | undefined { const node = ( @@ -892,7 +902,7 @@ export function transformCjsImport( ) { config.logger.warn( colors.yellow( - `\nUnable to interop \`${importExp}\` and may lose module exports. Please export "${rawUrl}" as ESM or use named exports instead, e.g. \`export { A, B } from "${rawUrl}"\``, + `\nUnable to interop \`${importExp}\` in ${importer}, this may lose module exports. Please export "${rawUrl}" as ESM or use named exports instead, e.g. \`export { A, B } from "${rawUrl}"\``, ), ) } else if ( diff --git a/packages/vite/src/node/plugins/importAnalysisBuild.ts b/packages/vite/src/node/plugins/importAnalysisBuild.ts index 35c638ad07f787..30ce2b7cbdf862 100644 --- a/packages/vite/src/node/plugins/importAnalysisBuild.ts +++ b/packages/vite/src/node/plugins/importAnalysisBuild.ts @@ -338,7 +338,14 @@ export function buildImportAnalysisPlugin(config: ResolvedConfig): Plugin { } } else if (needsInterop) { // config.logger.info(`${url} needs interop`) - interopNamedImports(str(), imports[index], url, index, config) + interopNamedImports( + str(), + imports[index], + url, + index, + importer, + config, + ) rewriteDone = true } if (!rewriteDone) {