From e2e44ff32b7b2bcbcae793cdcda263d1ad495fc0 Mon Sep 17 00:00:00 2001 From: Bjorn Lu Date: Sun, 5 Jun 2022 18:55:45 +0800 Subject: [PATCH] feat: handle named imports of builtin modules (#8338) --- .../src/node/optimizer/esbuildDepPlugin.ts | 36 ++++++++-- .../vite/src/node/plugins/importAnalysis.ts | 69 +++++++++++++------ packages/vite/src/node/plugins/resolve.ts | 16 +++-- playground/optimize-deps/.env | 1 - .../__tests__/optimize-deps.spec.ts | 41 ++++++++++- .../dep-with-builtin-module-cjs/index.js | 18 +++++ .../dep-with-builtin-module-cjs/package.json | 6 ++ .../dep-with-builtin-module-esm/index.js | 21 ++++++ .../dep-with-builtin-module-esm/package.json | 7 ++ playground/optimize-deps/index.html | 31 +++++++++ playground/optimize-deps/package.json | 2 + playground/optimize-deps/vite.config.js | 13 ++++ pnpm-lock.yaml | 22 ++++++ 13 files changed, 244 insertions(+), 39 deletions(-) delete mode 100644 playground/optimize-deps/.env create mode 100644 playground/optimize-deps/dep-with-builtin-module-cjs/index.js create mode 100644 playground/optimize-deps/dep-with-builtin-module-cjs/package.json create mode 100644 playground/optimize-deps/dep-with-builtin-module-esm/index.js create mode 100644 playground/optimize-deps/dep-with-builtin-module-esm/package.json diff --git a/packages/vite/src/node/optimizer/esbuildDepPlugin.ts b/packages/vite/src/node/optimizer/esbuildDepPlugin.ts index ee2a6e18cf04c6..fbfb658e9f7505 100644 --- a/packages/vite/src/node/optimizer/esbuildDepPlugin.ts +++ b/packages/vite/src/node/optimizer/esbuildDepPlugin.ts @@ -219,15 +219,37 @@ export function esbuildDepPlugin( build.onLoad( { filter: /.*/, namespace: 'browser-external' }, - ({ path: id }) => { + ({ path }) => { return { - contents: - `export default new Proxy({}, { - get() { - throw new Error('Module "${id}" has been externalized for ` + - `browser compatibility and cannot be accessed in client code.') + // Return in CJS to intercept named imports. Use `Object.create` to + // create the Proxy in the prototype to workaround esbuild issue. Why? + // + // In short, esbuild cjs->esm flow: + // 1. Create empty object using `Object.create(Object.getPrototypeOf(module.exports))`. + // 2. Assign props of `module.exports` to the object. + // 3. Return object for ESM use. + // + // If we do `module.exports = new Proxy({}, {})`, step 1 returns empty object, + // step 2 does nothing as there's no props for `module.exports`. The final object + // is just an empty object. + // + // Creating the Proxy in the prototype satisfies step 1 immediately, which means + // the returned object is a Proxy that we can intercept. + // + // Note: Skip keys that are accessed by esbuild and browser devtools. + contents: `\ +module.exports = Object.create(new Proxy({}, { + get(_, key) { + if ( + key !== '__esModule' && + key !== '__proto__' && + key !== 'constructor' && + key !== 'splice' + ) { + throw new Error(\`Module "${path}" has been externalized for browser compatibility. Cannot access "${path}.\${key}" in client code.\`) + } } -})` +}))` } } ) diff --git a/packages/vite/src/node/plugins/importAnalysis.ts b/packages/vite/src/node/plugins/importAnalysis.ts index ba221b4b1d5283..0c279da0601727 100644 --- a/packages/vite/src/node/plugins/importAnalysis.ts +++ b/packages/vite/src/node/plugins/importAnalysis.ts @@ -58,6 +58,7 @@ import { delayDepsOptimizerUntil } from './optimizedDeps' import { isCSSRequest, isDirectCSSRequest } from './css' +import { browserExternalId } from './resolve' const isDebug = !!process.env.DEBUG const debug = createDebugger('vite:import-analysis') @@ -322,11 +323,9 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { s: start, e: end, ss: expStart, - se: expEnd, d: dynamicIndex, // #2083 User may use escape path, // so use imports[index].n to get the unescaped string - // @ts-ignore n: specifier } = imports[index] @@ -434,29 +433,20 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { } } else if (needsInterop) { debug(`${url} needs interop`) - if (isDynamicImport) { - // rewrite `import('package')` to expose the default directly - str().overwrite( - expStart, - expEnd, - `import('${url}').then(m => m.default && m.default.__esModule ? m.default : ({ ...m.default, default: m.default }))`, - { contentOnly: true } - ) - } else { - const exp = source.slice(expStart, expEnd) - const rewritten = transformCjsImport(exp, url, rawUrl, index) - if (rewritten) { - str().overwrite(expStart, expEnd, rewritten, { - contentOnly: true - }) - } else { - // #1439 export * from '...' - str().overwrite(start, end, url, { contentOnly: true }) - } - } + interopNamedImports(str(), imports[index], url, index) rewriteDone = true } } + // If source code imports builtin modules via named imports, the stub proxy export + // would fail as it's `export default` only. Apply interop for builtin modules to + // correctly throw the error message. + else if ( + url.includes(browserExternalId) && + source.slice(expStart, start).includes('{') + ) { + interopNamedImports(str(), imports[index], url, index) + rewriteDone = true + } if (!rewriteDone) { str().overwrite(start, end, isDynamicImport ? `'${url}'` : url, { contentOnly: true @@ -639,6 +629,41 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { } } +function interopNamedImports( + str: MagicString, + importSpecifier: ImportSpecifier, + rewrittenUrl: string, + importIndex: number +) { + const source = str.original + const { + s: start, + e: end, + ss: expStart, + se: expEnd, + d: dynamicIndex + } = importSpecifier + if (dynamicIndex > -1) { + // rewrite `import('package')` to expose the default directly + str.overwrite( + expStart, + expEnd, + `import('${rewrittenUrl}').then(m => m.default && m.default.__esModule ? m.default : ({ ...m.default, default: m.default }))`, + { contentOnly: true } + ) + } else { + const exp = source.slice(expStart, expEnd) + const rawUrl = source.slice(start, end) + const rewritten = transformCjsImport(exp, rewrittenUrl, rawUrl, importIndex) + if (rewritten) { + str.overwrite(expStart, expEnd, rewritten, { contentOnly: true }) + } else { + // #1439 export * from '...' + str.overwrite(start, end, rewrittenUrl, { contentOnly: true }) + } + } +} + type ImportNameSpecifier = { importedName: string; localName: string } /** diff --git a/packages/vite/src/node/plugins/resolve.ts b/packages/vite/src/node/plugins/resolve.ts index 33a5ad31efe5b4..7ccc3a7eb5cb7a 100644 --- a/packages/vite/src/node/plugins/resolve.ts +++ b/packages/vite/src/node/plugins/resolve.ts @@ -339,15 +339,17 @@ export function resolvePlugin(baseOptions: InternalResolveOptions): Plugin { load(id) { if (id.startsWith(browserExternalId)) { - return isProduction - ? `export default {}` - : `export default new Proxy({}, { - get() { - throw new Error('Module "${id.slice( - browserExternalId.length + 1 - )}" has been externalized for browser compatibility and cannot be accessed in client code.') + if (isProduction) { + return `export default {}` + } else { + id = id.slice(browserExternalId.length + 1) + return `\ +export default new Proxy({}, { + get(_, key) { + throw new Error(\`Module "${id}" has been externalized for browser compatibility. Cannot access "${id}.\${key}" in client code.\`) } })` + } } } } diff --git a/playground/optimize-deps/.env b/playground/optimize-deps/.env deleted file mode 100644 index 995fca4af2ee24..00000000000000 --- a/playground/optimize-deps/.env +++ /dev/null @@ -1 +0,0 @@ -NODE_ENV=production \ No newline at end of file diff --git a/playground/optimize-deps/__tests__/optimize-deps.spec.ts b/playground/optimize-deps/__tests__/optimize-deps.spec.ts index 929455300ba555..bf4fee61e4e74c 100644 --- a/playground/optimize-deps/__tests__/optimize-deps.spec.ts +++ b/playground/optimize-deps/__tests__/optimize-deps.spec.ts @@ -1,4 +1,11 @@ -import { getColor, page } from '~utils' +import { + browserErrors, + browserLogs, + getColor, + isBuild, + isServe, + page +} from '~utils' test('default + named imports from cjs dep (react)', async () => { expect(await page.textContent('.cjs button')).toBe('count is 0') @@ -63,7 +70,7 @@ test('import * from optimized dep', async () => { }) test('import from dep with process.env.NODE_ENV', async () => { - expect(await page.textContent('.node-env')).toMatch(`prod`) + expect(await page.textContent('.node-env')).toMatch(isBuild ? 'prod' : 'dev') }) test('import from dep with .notjs files', async () => { @@ -113,3 +120,33 @@ test('import aliased package with colon', async () => { test('variable names are reused in different scripts', async () => { expect(await page.textContent('.reused-variable-names')).toBe('reused') }) + +test.runIf(isServe)('error on builtin modules usage', () => { + expect(browserLogs).toEqual( + expect.arrayContaining([ + // from dep-with-builtin-module-esm top-level try-catch + expect.stringContaining( + 'dep-with-builtin-module-esm Error: Module "fs" has been externalized for browser compatibility. Cannot access "fs.readFileSync" in client code.' + ), + expect.stringContaining( + 'dep-with-builtin-module-esm Error: Module "path" has been externalized for browser compatibility. Cannot access "path.join" in client code.' + ), + // from dep-with-builtin-module-cjs top-level try-catch + expect.stringContaining( + 'dep-with-builtin-module-cjs Error: Module "path" has been externalized for browser compatibility. Cannot access "path.join" in client code.' + ) + ]) + ) + + expect(browserErrors.map((error) => error.message)).toEqual( + expect.arrayContaining([ + // from user source code + 'Module "buffer" has been externalized for browser compatibility. Cannot access "buffer.Buffer" in client code.', + 'Module "child_process" has been externalized for browser compatibility. Cannot access "child_process.execSync" in client code.', + // from dep-with-builtin-module-esm read() + 'Module "fs" has been externalized for browser compatibility. Cannot access "fs.readFileSync" in client code.', + // from dep-with-builtin-module-esm read() + 'Module "fs" has been externalized for browser compatibility. Cannot access "fs.readFileSync" in client code.' + ]) + ) +}) diff --git a/playground/optimize-deps/dep-with-builtin-module-cjs/index.js b/playground/optimize-deps/dep-with-builtin-module-cjs/index.js new file mode 100644 index 00000000000000..920a0da0d97dda --- /dev/null +++ b/playground/optimize-deps/dep-with-builtin-module-cjs/index.js @@ -0,0 +1,18 @@ +const fs = require('fs') +const path = require('path') + +// NOTE: require destructure would error immediately because of how esbuild +// compiles it. There's no way around it as it's direct property access, which +// triggers the Proxy get trap. + +// access from default import +try { + path.join() +} catch (e) { + console.log('dep-with-builtin-module-cjs', e) +} + +// access from function +module.exports.read = () => { + return fs.readFileSync('test') +} diff --git a/playground/optimize-deps/dep-with-builtin-module-cjs/package.json b/playground/optimize-deps/dep-with-builtin-module-cjs/package.json new file mode 100644 index 00000000000000..8b39306140e804 --- /dev/null +++ b/playground/optimize-deps/dep-with-builtin-module-cjs/package.json @@ -0,0 +1,6 @@ +{ + "name": "dep-with-builtin-module-cjs", + "private": true, + "version": "0.0.0", + "main": "index.js" +} diff --git a/playground/optimize-deps/dep-with-builtin-module-esm/index.js b/playground/optimize-deps/dep-with-builtin-module-esm/index.js new file mode 100644 index 00000000000000..e3734720051aca --- /dev/null +++ b/playground/optimize-deps/dep-with-builtin-module-esm/index.js @@ -0,0 +1,21 @@ +import { readFileSync } from 'fs' +import path from 'path' + +// access from named import +try { + readFileSync() +} catch (e) { + console.log('dep-with-builtin-module-esm', e) +} + +// access from default import +try { + path.join() +} catch (e) { + console.log('dep-with-builtin-module-esm', e) +} + +// access from function +export function read() { + return readFileSync('test') +} diff --git a/playground/optimize-deps/dep-with-builtin-module-esm/package.json b/playground/optimize-deps/dep-with-builtin-module-esm/package.json new file mode 100644 index 00000000000000..e01897e5a51850 --- /dev/null +++ b/playground/optimize-deps/dep-with-builtin-module-esm/package.json @@ -0,0 +1,7 @@ +{ + "name": "dep-with-builtin-module-esm", + "private": true, + "version": "0.0.0", + "main": "index.js", + "type": "module" +} diff --git a/playground/optimize-deps/index.html b/playground/optimize-deps/index.html index c70ab5b2f09322..39d76fd83d0609 100644 --- a/playground/optimize-deps/index.html +++ b/playground/optimize-deps/index.html @@ -138,3 +138,34 @@

Reused variable names

const reusedName = 'reused' text('.reused-variable-names', reusedName) + + + + + + + + diff --git a/playground/optimize-deps/package.json b/playground/optimize-deps/package.json index d323ca77af0a29..7509b8284b9bc1 100644 --- a/playground/optimize-deps/package.json +++ b/playground/optimize-deps/package.json @@ -19,6 +19,8 @@ "dep-linked-include": "link:./dep-linked-include", "dep-node-env": "file:./dep-node-env", "dep-not-js": "file:./dep-not-js", + "dep-with-builtin-module-cjs": "file:./dep-with-builtin-module-cjs", + "dep-with-builtin-module-esm": "file:./dep-with-builtin-module-esm", "dep-with-dynamic-import": "file:./dep-with-dynamic-import", "lodash-es": "^4.17.21", "nested-exclude": "file:./nested-exclude", diff --git a/playground/optimize-deps/vite.config.js b/playground/optimize-deps/vite.config.js index fb3bbfc4a33eb5..d5da62f3331fc2 100644 --- a/playground/optimize-deps/vite.config.js +++ b/playground/optimize-deps/vite.config.js @@ -62,6 +62,19 @@ module.exports = { return { code } } } + }, + // TODO: Remove this one support for prebundling in build lands. + // It is expected that named importing in build doesn't work + // as it incurs a lot of overhead in build. + { + name: 'polyfill-named-fs-build', + apply: 'build', + enforce: 'pre', + load(id) { + if (id === '__vite-browser-external:fs') { + return `export default {}; export function readFileSync() {}` + } + } } ] } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index aa07fd3c5ae874..5e2530f0dafc6b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -582,6 +582,8 @@ importers: dep-linked-include: link:./dep-linked-include dep-node-env: file:./dep-node-env dep-not-js: file:./dep-not-js + dep-with-builtin-module-cjs: file:./dep-with-builtin-module-cjs + dep-with-builtin-module-esm: file:./dep-with-builtin-module-esm dep-with-dynamic-import: file:./dep-with-dynamic-import lodash-es: ^4.17.21 nested-exclude: file:./nested-exclude @@ -603,6 +605,8 @@ importers: dep-linked-include: link:dep-linked-include dep-node-env: file:playground/optimize-deps/dep-node-env dep-not-js: file:playground/optimize-deps/dep-not-js + dep-with-builtin-module-cjs: file:playground/optimize-deps/dep-with-builtin-module-cjs + dep-with-builtin-module-esm: file:playground/optimize-deps/dep-with-builtin-module-esm dep-with-dynamic-import: file:playground/optimize-deps/dep-with-dynamic-import lodash-es: 4.17.21 nested-exclude: file:playground/optimize-deps/nested-exclude @@ -646,6 +650,12 @@ importers: playground/optimize-deps/dep-not-js: specifiers: {} + playground/optimize-deps/dep-with-builtin-module-cjs: + specifiers: {} + + playground/optimize-deps/dep-with-builtin-module-esm: + specifiers: {} + playground/optimize-deps/dep-with-dynamic-import: specifiers: {} @@ -8839,6 +8849,18 @@ packages: version: 1.0.0 dev: false + file:playground/optimize-deps/dep-with-builtin-module-cjs: + resolution: {directory: playground/optimize-deps/dep-with-builtin-module-cjs, type: directory} + name: dep-with-builtin-module-cjs + version: 0.0.0 + dev: false + + file:playground/optimize-deps/dep-with-builtin-module-esm: + resolution: {directory: playground/optimize-deps/dep-with-builtin-module-esm, type: directory} + name: dep-with-builtin-module-esm + version: 0.0.0 + dev: false + file:playground/optimize-deps/dep-with-dynamic-import: resolution: {directory: playground/optimize-deps/dep-with-dynamic-import, type: directory} name: dep-with-dynamic-import