Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(importAnalysis): warning on ExportAllDeclaration #12799

Merged
merged 4 commits into from Apr 13, 2023
Merged

fix(importAnalysis): warning on ExportAllDeclaration #12799

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48d198a
fix(importAnalysis): warning on ExportAllDeclaration
sun0day Apr 9, 2023
360983d
test: export *
sun0day Apr 9, 2023
a2a72da
chore: rewrite warning message
sun0day Apr 13, 2023
7aeee40
feat: attach importer info
sun0day Apr 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 47 additions & 7 deletions packages/vite/src/node/__tests__/plugins/import.spec.ts
View file
Open in desktop
@@ -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(
Expand All @@ -12,6 +22,7 @@ describe('transformCjsImport', () => {
url,
rawUrl,
0,
config,
),
).toBe(
'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' +
Expand All @@ -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',
Expand All @@ -34,6 +45,7 @@ describe('transformCjsImport', () => {
url,
rawUrl,
0,
config,
),
).toBe(
'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' +
Expand All @@ -43,21 +55,39 @@ 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',
)
})

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', () => {
Expand All @@ -67,6 +97,7 @@ describe('transformCjsImport', () => {
url,
rawUrl,
0,
config,
),
).toBe(
'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' +
Expand All @@ -81,6 +112,7 @@ describe('transformCjsImport', () => {
url,
rawUrl,
0,
config,
),
).toBe(
'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' +
Expand All @@ -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; ' +
Expand All @@ -105,6 +143,7 @@ describe('transformCjsImport', () => {
url,
rawUrl,
0,
config,
),
).toBe(
'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' +
Expand All @@ -118,6 +157,7 @@ describe('transformCjsImport', () => {
url,
rawUrl,
0,
config,
),
).toBe(
'import __vite__cjsImport0_react from "./node_modules/.vite/deps/react.js"; ' +
Expand Down
32 changes: 29 additions & 3 deletions packages/vite/src/node/plugins/importAnalysis.ts
View file
Open in desktop
Expand Up @@ -585,7 +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,
)
rewriteDone = true
}
}
Expand All @@ -596,7 +602,7 @@ 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)
rewriteDone = true
}
if (!rewriteDone) {
Expand Down Expand Up @@ -811,6 +817,8 @@ export function interopNamedImports(
importSpecifier: ImportSpecifier,
rewrittenUrl: string,
importIndex: number,

config: ResolvedConfig,
): void {
const source = str.original
const {
Expand All @@ -831,7 +839,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,
config,
)
if (rewritten) {
str.overwrite(expStart, expEnd, rewritten, { contentOnly: true })
} else {
Expand Down Expand Up @@ -861,6 +875,7 @@ export function transformCjsImport(
url: string,
rawUrl: string,
importIndex: number,
config: ResolvedConfig,
): string | undefined {
const node = (
parseJS(importExp, {
Expand All @@ -869,7 +884,18 @@ export function transformCjsImport(
}) as any
).body[0] as Node

// `export * from '...'` may cause unexpected problem, so give it a warning
if (
config.command === 'serve' &&
node.type === 'ExportAllDeclaration' &&
!node.exported
) {
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.`,
bluwy marked this conversation as resolved.
Show resolved Hide resolved
),
)
} else if (
node.type === 'ImportDeclaration' ||
node.type === 'ExportNamedDeclaration'
) {
Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/plugins/importAnalysisBuild.ts
View file
Open in desktop
Expand Up @@ -338,7 +338,7 @@ 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)
rewriteDone = true
}
if (!rewriteDone) {
Expand Down