From 3a543dfd9142f936b0157893580c25f4cb729b0f Mon Sep 17 00:00:00 2001 From: Tom Jenkinson Date: Thu, 22 Apr 2021 14:25:07 +0100 Subject: [PATCH] fix(node-resolve)!: mark module as external if resolved module is external (#799) BREAKING CHANGES: Now requires rollup@^2.42.0 * fix(node-resolve): mark module as external if resolved module is external fixes #609 * don't use ?. * prevent infinite loops * check both importee and importer in nested this.resolve and add more tests * upgrade to rollup 2.42.0 to simplify nested `this.resolve` --- packages/node-resolve/package.json | 4 +- packages/node-resolve/src/index.js | 340 +++++++++--------- .../test/fixtures/resolved-external/main.js | 1 + .../node_modules/external/index.js | 1 + packages/node-resolve/test/test.js | 12 + pnpm-lock.yaml | 54 ++- 6 files changed, 234 insertions(+), 178 deletions(-) create mode 100755 packages/node-resolve/test/fixtures/resolved-external/main.js create mode 100644 packages/node-resolve/test/fixtures/resolved-external/node_modules/external/index.js diff --git a/packages/node-resolve/package.json b/packages/node-resolve/package.json index 78cf2c239..e80fac5fd 100644 --- a/packages/node-resolve/package.json +++ b/packages/node-resolve/package.json @@ -54,7 +54,7 @@ "modules" ], "peerDependencies": { - "rollup": "^1.20.0||^2.0.0" + "rollup": "^2.42.0" }, "dependencies": { "@rollup/pluginutils": "^3.1.0", @@ -71,7 +71,7 @@ "@rollup/plugin-commonjs": "^16.0.0", "@rollup/plugin-json": "^4.1.0", "es5-ext": "^0.10.53", - "rollup": "^2.23.0", + "rollup": "^2.42.0", "source-map": "^0.7.3", "string-capitalize": "^1.0.1" }, diff --git a/packages/node-resolve/src/index.js b/packages/node-resolve/src/index.js index a1861864b..5b342ca5f 100644 --- a/packages/node-resolve/src/index.js +++ b/packages/node-resolve/src/index.js @@ -72,203 +72,215 @@ export function nodeResolve(opts = {}) { const browserMapCache = new Map(); let preserveSymlinks; - return { - name: 'node-resolve', + const doResolveId = async (context, importee, importer, opts) => { + // strip query params from import + const [importPath, params] = importee.split('?'); + const importSuffix = `${params ? `?${params}` : ''}`; + importee = importPath; + + const baseDir = !importer || dedupe(importee) ? rootDir : dirname(importer); + + // https://github.com/defunctzombie/package-browser-field-spec + const browser = browserMapCache.get(importer); + if (useBrowserOverrides && browser) { + const resolvedImportee = resolve(baseDir, importee); + if (browser[importee] === false || browser[resolvedImportee] === false) { + return { id: ES6_BROWSER_EMPTY }; + } + const browserImportee = + browser[importee] || + browser[resolvedImportee] || + browser[`${resolvedImportee}.js`] || + browser[`${resolvedImportee}.json`]; + if (browserImportee) { + importee = browserImportee; + } + } - buildStart(options) { - rollupOptions = options; + const parts = importee.split(/[/\\]/); + let id = parts.shift(); + let isRelativeImport = false; + + if (id[0] === '@' && parts.length > 0) { + // scoped packages + id += `/${parts.shift()}`; + } else if (id[0] === '.') { + // an import relative to the parent dir of the importer + id = resolve(baseDir, importee); + isRelativeImport = true; + } - for (const warning of warnings) { - this.warn(warning); + if ( + !isRelativeImport && + resolveOnly.length && + !resolveOnly.some((pattern) => pattern.test(id)) + ) { + if (normalizeInput(rollupOptions.input).includes(importee)) { + return null; } + return false; + } - ({ preserveSymlinks } = options); - }, + const importSpecifierList = []; - generateBundle() { - readCachedFile.clear(); - isFileCached.clear(); - isDirCached.clear(); - }, + if (importer === undefined && !importee[0].match(/^\.?\.?\//)) { + // For module graph roots (i.e. when importer is undefined), we + // need to handle 'path fragments` like `foo/bar` that are commonly + // found in rollup config files. If importee doesn't look like a + // relative or absolute path, we make it relative and attempt to + // resolve it. If we don't find anything, we try resolving it as we + // got it. + importSpecifierList.push(`./${importee}`); + } - async resolveId(importee, importer, opts) { - if (importee === ES6_BROWSER_EMPTY) { - return importee; - } - // ignore IDs with null character, these belong to other plugins - if (/\0/.test(importee)) return null; + const importeeIsBuiltin = builtins.has(importee); - if (/\0/.test(importer)) { - importer = undefined; - } + if (importeeIsBuiltin) { + // The `resolve` library will not resolve packages with the same + // name as a node built-in module. If we're resolving something + // that's a builtin, and we don't prefer to find built-ins, we + // first try to look up a local module with that name. If we don't + // find anything, we resolve the builtin which just returns back + // the built-in's name. + importSpecifierList.push(`${importee}/`); + } - // strip query params from import - const [importPath, params] = importee.split('?'); - const importSuffix = `${params ? `?${params}` : ''}`; - importee = importPath; + // TypeScript files may import '.js' to refer to either '.ts' or '.tsx' + if (importer && importee.endsWith('.js')) { + for (const ext of ['.ts', '.tsx']) { + if (importer.endsWith(ext) && extensions.includes(ext)) { + importSpecifierList.push(importee.replace(/.js$/, ext)); + } + } + } - const baseDir = !importer || dedupe(importee) ? rootDir : dirname(importer); + importSpecifierList.push(importee); + + const warn = (...args) => context.warn(...args); + const isRequire = + opts && opts.custom && opts.custom['node-resolve'] && opts.custom['node-resolve'].isRequire; + const exportConditions = isRequire ? conditionsCjs : conditionsEsm; + + const resolvedWithoutBuiltins = await resolveImportSpecifiers({ + importer, + importSpecifierList, + exportConditions, + warn, + packageInfoCache, + extensions, + mainFields, + preserveSymlinks, + useBrowserOverrides, + baseDir, + moduleDirectories, + rootDir, + ignoreSideEffectsForRoot + }); + + const resolved = + importeeIsBuiltin && preferBuiltins + ? { + packageInfo: undefined, + hasModuleSideEffects: () => null, + hasPackageEntry: true, + packageBrowserField: false + } + : resolvedWithoutBuiltins; + if (!resolved) { + return null; + } - // https://github.com/defunctzombie/package-browser-field-spec - const browser = browserMapCache.get(importer); - if (useBrowserOverrides && browser) { - const resolvedImportee = resolve(baseDir, importee); - if (browser[importee] === false || browser[resolvedImportee] === false) { - return ES6_BROWSER_EMPTY; - } - const browserImportee = - browser[importee] || - browser[resolvedImportee] || - browser[`${resolvedImportee}.js`] || - browser[`${resolvedImportee}.json`]; - if (browserImportee) { - importee = browserImportee; + const { packageInfo, hasModuleSideEffects, hasPackageEntry, packageBrowserField } = resolved; + let { location } = resolved; + if (packageBrowserField) { + if (Object.prototype.hasOwnProperty.call(packageBrowserField, location)) { + if (!packageBrowserField[location]) { + browserMapCache.set(location, packageBrowserField); + return { id: ES6_BROWSER_EMPTY }; } + location = packageBrowserField[location]; } + browserMapCache.set(location, packageBrowserField); + } - const parts = importee.split(/[/\\]/); - let id = parts.shift(); - let isRelativeImport = false; - - if (id[0] === '@' && parts.length > 0) { - // scoped packages - id += `/${parts.shift()}`; - } else if (id[0] === '.') { - // an import relative to the parent dir of the importer - id = resolve(baseDir, importee); - isRelativeImport = true; + if (hasPackageEntry && !preserveSymlinks) { + const fileExists = await exists(location); + if (fileExists) { + location = await realpath(location); } + } - if ( - !isRelativeImport && - resolveOnly.length && - !resolveOnly.some((pattern) => pattern.test(id)) - ) { - if (normalizeInput(rollupOptions.input).includes(importee)) { - return null; + idToPackageInfo.set(location, packageInfo); + + if (hasPackageEntry) { + if (importeeIsBuiltin && preferBuiltins) { + if (!isPreferBuiltinsSet && resolvedWithoutBuiltins && resolved !== importee) { + context.warn( + `preferring built-in module '${importee}' over local alternative at '${resolvedWithoutBuiltins.location}', pass 'preferBuiltins: false' to disable this behavior or 'preferBuiltins: true' to disable this warning` + ); } return false; + } else if (jail && location.indexOf(normalize(jail.trim(sep))) !== 0) { + return null; } + } - const importSpecifierList = []; - - if (importer === undefined && !importee[0].match(/^\.?\.?\//)) { - // For module graph roots (i.e. when importer is undefined), we - // need to handle 'path fragments` like `foo/bar` that are commonly - // found in rollup config files. If importee doesn't look like a - // relative or absolute path, we make it relative and attempt to - // resolve it. If we don't find anything, we try resolving it as we - // got it. - importSpecifierList.push(`./${importee}`); + if (options.modulesOnly && (await exists(location))) { + const code = await readFile(location, 'utf-8'); + if (isModule(code)) { + return { + id: `${location}${importSuffix}`, + moduleSideEffects: hasModuleSideEffects(location) + }; } + return null; + } + const result = { + id: `${location}${importSuffix}`, + moduleSideEffects: hasModuleSideEffects(location) + }; + return result; + }; - const importeeIsBuiltin = builtins.has(importee); + return { + name: 'node-resolve', - if (importeeIsBuiltin) { - // The `resolve` library will not resolve packages with the same - // name as a node built-in module. If we're resolving something - // that's a builtin, and we don't prefer to find built-ins, we - // first try to look up a local module with that name. If we don't - // find anything, we resolve the builtin which just returns back - // the built-in's name. - importSpecifierList.push(`${importee}/`); - } + buildStart(options) { + rollupOptions = options; - // TypeScript files may import '.js' to refer to either '.ts' or '.tsx' - if (importer && importee.endsWith('.js')) { - for (const ext of ['.ts', '.tsx']) { - if (importer.endsWith(ext) && extensions.includes(ext)) { - importSpecifierList.push(importee.replace(/.js$/, ext)); - } - } + for (const warning of warnings) { + this.warn(warning); } - importSpecifierList.push(importee); - - const warn = (...args) => this.warn(...args); - const isRequire = - opts && opts.custom && opts.custom['node-resolve'] && opts.custom['node-resolve'].isRequire; - const exportConditions = isRequire ? conditionsCjs : conditionsEsm; - - const resolvedWithoutBuiltins = await resolveImportSpecifiers({ - importer, - importSpecifierList, - exportConditions, - warn, - packageInfoCache, - extensions, - mainFields, - preserveSymlinks, - useBrowserOverrides, - baseDir, - moduleDirectories, - rootDir, - ignoreSideEffectsForRoot - }); - - const resolved = - importeeIsBuiltin && preferBuiltins - ? { - packageInfo: undefined, - hasModuleSideEffects: () => null, - hasPackageEntry: true, - packageBrowserField: false - } - : resolvedWithoutBuiltins; - if (!resolved) { - return null; - } + ({ preserveSymlinks } = options); + }, - const { packageInfo, hasModuleSideEffects, hasPackageEntry, packageBrowserField } = resolved; - let { location } = resolved; - if (packageBrowserField) { - if (Object.prototype.hasOwnProperty.call(packageBrowserField, location)) { - if (!packageBrowserField[location]) { - browserMapCache.set(location, packageBrowserField); - return ES6_BROWSER_EMPTY; - } - location = packageBrowserField[location]; - } - browserMapCache.set(location, packageBrowserField); - } + generateBundle() { + readCachedFile.clear(); + isFileCached.clear(); + isDirCached.clear(); + }, - if (hasPackageEntry && !preserveSymlinks) { - const fileExists = await exists(location); - if (fileExists) { - location = await realpath(location); - } + async resolveId(importee, importer, opts) { + if (importee === ES6_BROWSER_EMPTY) { + return importee; } + // ignore IDs with null character, these belong to other plugins + if (/\0/.test(importee)) return null; - idToPackageInfo.set(location, packageInfo); - - if (hasPackageEntry) { - if (importeeIsBuiltin && preferBuiltins) { - if (!isPreferBuiltinsSet && resolvedWithoutBuiltins && resolved !== importee) { - this.warn( - `preferring built-in module '${importee}' over local alternative at '${resolvedWithoutBuiltins.location}', pass 'preferBuiltins: false' to disable this behavior or 'preferBuiltins: true' to disable this warning` - ); - } - return false; - } else if (jail && location.indexOf(normalize(jail.trim(sep))) !== 0) { - return null; - } + if (/\0/.test(importer)) { + importer = undefined; } - if (options.modulesOnly && (await exists(location))) { - const code = await readFile(location, 'utf-8'); - if (isModule(code)) { - return { - id: `${location}${importSuffix}`, - moduleSideEffects: hasModuleSideEffects(location) - }; + const resolved = await doResolveId(this, importee, importer, opts); + if (resolved) { + const resolvedResolved = await this.resolve(resolved.id, importer, { skipSelf: true }); + const isExternal = !!(resolvedResolved && resolvedResolved.external); + if (isExternal) { + return false; } - return null; } - const result = { - id: `${location}${importSuffix}`, - moduleSideEffects: hasModuleSideEffects(location) - }; - return result; + return resolved; }, load(importee) { diff --git a/packages/node-resolve/test/fixtures/resolved-external/main.js b/packages/node-resolve/test/fixtures/resolved-external/main.js new file mode 100755 index 000000000..b2bc48d5b --- /dev/null +++ b/packages/node-resolve/test/fixtures/resolved-external/main.js @@ -0,0 +1 @@ +import 'external'; diff --git a/packages/node-resolve/test/fixtures/resolved-external/node_modules/external/index.js b/packages/node-resolve/test/fixtures/resolved-external/node_modules/external/index.js new file mode 100644 index 000000000..addbaebd2 --- /dev/null +++ b/packages/node-resolve/test/fixtures/resolved-external/node_modules/external/index.js @@ -0,0 +1 @@ +console.log('external'); diff --git a/packages/node-resolve/test/test.js b/packages/node-resolve/test/test.js index 7a279a467..9fd475a69 100755 --- a/packages/node-resolve/test/test.js +++ b/packages/node-resolve/test/test.js @@ -363,6 +363,18 @@ test('can resolve imports with search params and hash', async (t) => { t.is(module.exports, 'resolved with search params and hash'); }); +test('marks a module as external if the resolved version is external', async (t) => { + const bundle = await rollup({ + input: 'resolved-external/main.js', + onwarn: () => t.fail('No warnings were expected'), + external: [/node_modules/], + plugins: [nodeResolve()] + }); + + const code = await getCode(bundle); + t.is(/node_modules/.test(code), false); +}); + [ 'preserveSymlinks', 'basedir', diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3c62f4cf0..d0c32c8eb 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -333,7 +333,7 @@ importers: rollup: ^2.23.0 packages/node-resolve: dependencies: - '@rollup/pluginutils': 3.1.0_rollup@2.32.1 + '@rollup/pluginutils': 3.1.0_rollup@2.42.0 '@types/resolve': 1.17.1 builtin-modules: 3.1.0 deepmerge: 4.2.2 @@ -342,11 +342,11 @@ importers: devDependencies: '@babel/core': 7.12.3 '@babel/plugin-transform-typescript': 7.12.1_@babel+core@7.12.3 - '@rollup/plugin-babel': 5.2.1_@babel+core@7.12.3+rollup@2.32.1 - '@rollup/plugin-commonjs': 16.0.0_rollup@2.32.1 - '@rollup/plugin-json': 4.1.0_rollup@2.32.1 + '@rollup/plugin-babel': 5.2.1_@babel+core@7.12.3+rollup@2.42.0 + '@rollup/plugin-commonjs': 16.0.0_rollup@2.42.0 + '@rollup/plugin-json': 4.1.0_rollup@2.42.0 es5-ext: 0.10.53 - rollup: 2.32.1 + rollup: 2.42.0 source-map: 0.7.3 string-capitalize: 1.0.1 specifiers: @@ -362,7 +362,7 @@ importers: es5-ext: ^0.10.53 is-module: ^1.0.0 resolve: ^1.19.0 - rollup: ^2.23.0 + rollup: ^2.42.0 source-map: ^0.7.3 string-capitalize: ^1.0.1 packages/pluginutils: @@ -1635,12 +1635,12 @@ packages: rollup: ^1.20.0||^2.0.0 resolution: integrity: sha512-hNcQY4bpBUIvxekd26DBPgF7BT4mKVNDF5tBG4Zi+3IgwLxGYRY0itHs9D0oLVwXM5pvJDWJlBQro+au8WaUWw== - /@rollup/plugin-babel/5.2.1_@babel+core@7.12.3+rollup@2.32.1: + /@rollup/plugin-babel/5.2.1_@babel+core@7.12.3+rollup@2.42.0: dependencies: '@babel/core': 7.12.3 '@babel/helper-module-imports': 7.12.1 - '@rollup/pluginutils': 3.1.0_rollup@2.32.1 - rollup: 2.32.1 + '@rollup/pluginutils': 3.1.0_rollup@2.42.0 + rollup: 2.42.0 dev: true engines: node: '>= 10.0.0' @@ -1700,16 +1700,16 @@ packages: rollup: ^2.3.4 resolution: integrity: sha512-+PSmD9ePwTAeU106i9FRdc+Zb3XUWyW26mo5Atr2mk82hor8+nPwkztEjFo8/B1fJKfaQDg9aM2bzQkjhi7zOw== - /@rollup/plugin-commonjs/16.0.0_rollup@2.32.1: + /@rollup/plugin-commonjs/16.0.0_rollup@2.42.0: dependencies: - '@rollup/pluginutils': 3.1.0_rollup@2.32.1 + '@rollup/pluginutils': 3.1.0_rollup@2.42.0 commondir: 1.0.1 estree-walker: 2.0.1 glob: 7.1.6 is-reference: 1.2.1 magic-string: 0.25.7 resolve: 1.18.1 - rollup: 2.32.1 + rollup: 2.42.0 dev: true engines: node: '>= 8.0.0' @@ -1726,6 +1726,15 @@ packages: rollup: ^1.20.0 || ^2.0.0 resolution: integrity: sha512-yfLbTdNS6amI/2OpmbiBoW12vngr5NW2jCJVZSBEz+H5KfUJZ2M7sDjk0U6GOOdCWFVScShte29o9NezJ53TPw== + /@rollup/plugin-json/4.1.0_rollup@2.42.0: + dependencies: + '@rollup/pluginutils': 3.1.0_rollup@2.42.0 + rollup: 2.42.0 + dev: true + peerDependencies: + rollup: ^1.20.0 || ^2.0.0 + resolution: + integrity: sha512-yfLbTdNS6amI/2OpmbiBoW12vngr5NW2jCJVZSBEz+H5KfUJZ2M7sDjk0U6GOOdCWFVScShte29o9NezJ53TPw== /@rollup/plugin-node-resolve/10.0.0_rollup@2.32.1: dependencies: '@rollup/pluginutils': 3.1.0_rollup@2.32.1 @@ -1855,6 +1864,18 @@ packages: rollup: ^1.20.0||^2.0.0 resolution: integrity: sha512-GksZ6pr6TpIjHm8h9lSQ8pi8BE9VeubNT0OMJ3B5uZJ8pz73NPiqOtCog/x2/QzM1ENChPKxMDhiQuRHsqc+lg== + /@rollup/pluginutils/3.1.0_rollup@2.42.0: + dependencies: + '@types/estree': 0.0.39 + estree-walker: 1.0.1 + picomatch: 2.2.2 + rollup: 2.42.0 + engines: + node: '>= 8.0.0' + peerDependencies: + rollup: ^1.20.0||^2.0.0 + resolution: + integrity: sha512-GksZ6pr6TpIjHm8h9lSQ8pi8BE9VeubNT0OMJ3B5uZJ8pz73NPiqOtCog/x2/QzM1ENChPKxMDhiQuRHsqc+lg== /@rollup/pluginutils/4.1.0_rollup@2.32.1: dependencies: estree-walker: 2.0.1 @@ -6712,6 +6733,15 @@ packages: fsevents: 2.1.3 resolution: integrity: sha512-Op2vWTpvK7t6/Qnm1TTh7VjEZZkN8RWgf0DHbkKzQBwNf748YhXbozHVefqpPp/Fuyk/PQPAnYsBxAEtlMvpUw== + /rollup/2.42.0: + dev: true + engines: + node: '>=10.0.0' + hasBin: true + optionalDependencies: + fsevents: 2.3.1 + resolution: + integrity: sha512-P9bJnaZ2P0hawoJo+Jto8YZZqil9URogNVE4KJeyj6wrUSDIbdMvmj7CsyEFwdXu/I5SiWEzB1hfmLeMldH6ww== /run-parallel/1.1.9: resolution: integrity: sha512-DEqnSRTDw/Tc3FXf49zedI638Z9onwUotBMiUFKmrO2sdFKIbXamXGQ3Axd4qgphxKB4kw/qP1w5kTxnfU1B9Q==