From 68fd2ac9993f96219c240f6ef0d9cd3e94cbf48d Mon Sep 17 00:00:00 2001 From: Gabriel Bota <94833492+dygabo@users.noreply.github.com> Date: Wed, 22 Dec 2021 19:43:19 +0100 Subject: [PATCH] loader: fix package resolution for edge case this commit solves a regression introduced with PR-40980. if a resolve call results in a script with .mjs extension the is automatically set to . This avoids the case where an additional in the same directory as the .mjs file would declare the to commonjs PR-URL: https://github.com/nodejs/node/pull/41218 Refs: https://github.com/nodejs/node/pull/40980 Refs: https://github.com/yargs/yargs/issues/2068 Reviewed-By: Guy Bedford Reviewed-By: Antoine du Hamel Reviewed-By: James M Snell --- lib/internal/modules/cjs/loader.js | 4 +- lib/internal/modules/esm/get_format.js | 66 ++++--- lib/internal/modules/esm/load.js | 3 +- lib/internal/modules/esm/resolve.js | 99 +++++----- test/es-module/test-esm-resolve-type.js | 230 +++++++++++++++--------- 5 files changed, 233 insertions(+), 169 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 63f224acb5c184..e0f40ffa2ecf50 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -457,7 +457,7 @@ function trySelf(parentPath, request) { try { return finalizeEsmResolution(packageExportsResolve( pathToFileURL(pkgPath + '/package.json'), expansion, pkg, - pathToFileURL(parentPath), cjsConditions).resolved, parentPath, pkgPath); + pathToFileURL(parentPath), cjsConditions), parentPath, pkgPath); } catch (e) { if (e.code === 'ERR_MODULE_NOT_FOUND') throw createEsmNotFoundErr(request, pkgPath + '/package.json'); @@ -481,7 +481,7 @@ function resolveExports(nmPath, request) { try { return finalizeEsmResolution(packageExportsResolve( pathToFileURL(pkgPath + '/package.json'), '.' + expansion, pkg, null, - cjsConditions).resolved, null, pkgPath); + cjsConditions), null, pkgPath); } catch (e) { if (e.code === 'ERR_MODULE_NOT_FOUND') throw createEsmNotFoundErr(request, pkgPath + '/package.json'); diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js index 7c07e5b1f7ce17..175988b17446d8 100644 --- a/lib/internal/modules/esm/get_format.js +++ b/lib/internal/modules/esm/get_format.js @@ -32,6 +32,8 @@ const legacyExtensionFormatMap = { '.node': 'commonjs' }; +let experimentalSpecifierResolutionWarned = false; + if (experimentalWasmModules) extensionFormatMap['.wasm'] = legacyExtensionFormatMap['.wasm'] = 'wasm'; @@ -53,41 +55,57 @@ const protocolHandlers = ObjectAssign(ObjectCreate(null), { return format; }, - 'file:'(parsed, url) { - const ext = extname(parsed.pathname); - let format; - - if (ext === '.js') { - format = getPackageType(parsed.href) === 'module' ? 'module' : 'commonjs'; - } else { - format = extensionFormatMap[ext]; - } - if (!format) { - if (experimentalSpecifierResolution === 'node') { - process.emitWarning( - 'The Node.js specifier resolution in ESM is experimental.', - 'ExperimentalWarning'); - format = legacyExtensionFormatMap[ext]; - } else { - throw new ERR_UNKNOWN_FILE_EXTENSION(ext, fileURLToPath(url)); - } - } - - return format || null; - }, + 'file:': getFileProtocolModuleFormat, 'node:'() { return 'builtin'; }, }); -function defaultGetFormat(url, context) { +function getLegacyExtensionFormat(ext) { + if ( + experimentalSpecifierResolution === 'node' && + !experimentalSpecifierResolutionWarned + ) { + process.emitWarning( + 'The Node.js specifier resolution in ESM is experimental.', + 'ExperimentalWarning'); + experimentalSpecifierResolutionWarned = true; + } + return legacyExtensionFormatMap[ext]; +} + +function getFileProtocolModuleFormat(url, ignoreErrors) { + const ext = extname(url.pathname); + if (ext === '.js') { + return getPackageType(url) === 'module' ? 'module' : 'commonjs'; + } + + const format = extensionFormatMap[ext]; + if (format) return format; + if (experimentalSpecifierResolution !== 'node') { + // Explicit undefined return indicates load hook should rerun format check + if (ignoreErrors) + return undefined; + throw new ERR_UNKNOWN_FILE_EXTENSION(ext, fileURLToPath(url)); + } + return getLegacyExtensionFormat(ext) ?? null; +} + +function defaultGetFormatWithoutErrors(url, context) { const parsed = new URL(url); + if (!ObjectPrototypeHasOwnProperty(protocolHandlers, parsed.protocol)) + return null; + return protocolHandlers[parsed.protocol](parsed, true); +} +function defaultGetFormat(url, context) { + const parsed = new URL(url); return ObjectPrototypeHasOwnProperty(protocolHandlers, parsed.protocol) ? - protocolHandlers[parsed.protocol](parsed, url) : + protocolHandlers[parsed.protocol](parsed, false) : null; } module.exports = { defaultGetFormat, + defaultGetFormatWithoutErrors, extensionFormatMap, legacyExtensionFormatMap, }; diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index 67123792e8903a..86fe0a77406ecf 100644 --- a/lib/internal/modules/esm/load.js +++ b/lib/internal/modules/esm/load.js @@ -2,7 +2,6 @@ const { defaultGetFormat } = require('internal/modules/esm/get_format'); const { defaultGetSource } = require('internal/modules/esm/get_source'); -const { translators } = require('internal/modules/esm/translators'); const { validateAssertions } = require('internal/modules/esm/assert'); /** @@ -18,7 +17,7 @@ async function defaultLoad(url, context) { } = context; const { importAssertions } = context; - if (!format || !translators.has(format)) { + if (format == null) { format = defaultGetFormat(url); } diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 5bd1d23c096bfb..e1b185d782830f 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -107,7 +107,7 @@ function emitTrailingSlashPatternDeprecation(match, pjsonUrl, base) { * @returns {void} */ function emitLegacyIndexDeprecation(url, packageJSONUrl, base, main) { - const format = defaultGetFormat(url); + const format = defaultGetFormatWithoutErrors(url); if (format !== 'module') return; const path = fileURLToPath(url); @@ -464,22 +464,6 @@ const patternRegEx = /\*/g; function resolvePackageTargetString( target, subpath, match, packageJSONUrl, base, pattern, internal, conditions) { - const composeResult = (resolved) => { - let format; - try { - format = getPackageType(resolved); - } catch (err) { - if (err.code === 'ERR_INVALID_FILE_URL_PATH') { - const invalidModuleErr = new ERR_INVALID_MODULE_SPECIFIER( - resolved, 'must not include encoded "/" or "\\" characters', base); - invalidModuleErr.cause = err; - throw invalidModuleErr; - } - throw err; - } - return { resolved, ...(format !== 'none') && { format } }; - }; - if (subpath !== '' && !pattern && target[target.length - 1] !== '/') throwInvalidPackageTarget(match, target, packageJSONUrl, internal, base); @@ -512,7 +496,7 @@ function resolvePackageTargetString( if (!StringPrototypeStartsWith(resolvedPath, packagePath)) throwInvalidPackageTarget(match, target, packageJSONUrl, internal, base); - if (subpath === '') return composeResult(resolved); + if (subpath === '') return resolved; if (RegExpPrototypeTest(invalidSegmentRegEx, subpath)) { const request = pattern ? @@ -521,12 +505,16 @@ function resolvePackageTargetString( } if (pattern) { - return composeResult(new URL(RegExpPrototypeSymbolReplace(patternRegEx, - resolved.href, - () => subpath))); + return new URL( + RegExpPrototypeSymbolReplace( + patternRegEx, + resolved.href, + () => subpath + ) + ); } - return composeResult(new URL(subpath, resolved)); + return new URL(subpath, resolved); } /** @@ -753,7 +741,7 @@ function packageImportsResolve(name, base, conditions) { packageJSONUrl, imports[name], '', name, base, false, true, conditions ); if (resolveResult != null) { - return resolveResult.resolved; + return resolveResult; } } else { let bestMatch = ''; @@ -785,7 +773,7 @@ function packageImportsResolve(name, base, conditions) { bestMatch, base, true, true, conditions); if (resolveResult != null) { - return resolveResult.resolved; + return resolveResult; } } } @@ -849,7 +837,7 @@ function parsePackageName(specifier, base) { */ function packageResolve(specifier, base, conditions) { if (NativeModule.canBeRequiredByUsers(specifier)) - return { resolved: new URL('node:' + specifier) }; + return new URL('node:' + specifier); const { packageName, packageSubpath, isScoped } = parsePackageName(specifier, base); @@ -888,19 +876,14 @@ function packageResolve(specifier, base, conditions) { packageJSONUrl, packageSubpath, packageConfig, base, conditions); } if (packageSubpath === '.') { - return { - resolved: legacyMainResolve( - packageJSONUrl, - packageConfig, - base), - ...(packageConfig.type !== 'none') && { format: packageConfig.type } - }; + return legacyMainResolve( + packageJSONUrl, + packageConfig, + base + ); } - return { - resolved: new URL(packageSubpath, packageJSONUrl), - ...(packageConfig.type !== 'none') && { format: packageConfig.type } - }; + return new URL(packageSubpath, packageJSONUrl); // Cross-platform root check. } while (packageJSONPath.length !== lastPath.length); @@ -944,7 +927,6 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks) { // Order swapped from spec for minor perf gain. // Ok since relative URLs cannot parse as URLs. let resolved; - let format; if (shouldBeTreatedAsRelativeOrAbsolutePath(specifier)) { resolved = new URL(specifier, base); } else if (specifier[0] === '#') { @@ -953,19 +935,13 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks) { try { resolved = new URL(specifier); } catch { - ({ resolved, format } = packageResolve(specifier, base, conditions)); + resolved = packageResolve(specifier, base, conditions); } } if (resolved.protocol !== 'file:') { - return { - url: resolved - }; + return resolved; } - - return { - url: finalizeResolution(resolved, base, preserveSymlinks), - ...(format != null) && { format } - }; + return finalizeResolution(resolved, base, preserveSymlinks); } /** @@ -1014,6 +990,13 @@ function resolveAsCommonJS(specifier, parentURL) { } } +function throwIfUnsupportedURLProtocol(url) { + if (url.protocol !== 'file:' && url.protocol !== 'data:' && + url.protocol !== 'node:') { + throw new ERR_UNSUPPORTED_ESM_URL_SCHEME(url); + } +} + function defaultResolve(specifier, context = {}, defaultResolveUnused) { let { parentURL, conditions } = context; if (parentURL && policy?.manifest) { @@ -1054,15 +1037,13 @@ function defaultResolve(specifier, context = {}, defaultResolveUnused) { conditions = getConditionsSet(conditions); let url; - let format; try { - ({ url, format } = - moduleResolve( - specifier, - parentURL, - conditions, - isMain ? preserveSymlinksMain : preserveSymlinks - )); + url = moduleResolve( + specifier, + parentURL, + conditions, + isMain ? preserveSymlinksMain : preserveSymlinks + ); } catch (error) { // Try to give the user a hint of what would have been the // resolved CommonJS module @@ -1086,13 +1067,11 @@ function defaultResolve(specifier, context = {}, defaultResolveUnused) { throw error; } - if (url.protocol !== 'file:' && url.protocol !== 'data:' && - url.protocol !== 'node:') - throw new ERR_UNSUPPORTED_ESM_URL_SCHEME(url); + throwIfUnsupportedURLProtocol(url); return { url: `${url}`, - ...(format != null) && { format } + format: defaultGetFormatWithoutErrors(url), }; } @@ -1107,4 +1086,6 @@ module.exports = { }; // cycle -const { defaultGetFormat } = require('internal/modules/esm/get_format'); +const { + defaultGetFormatWithoutErrors, +} = require('internal/modules/esm/get_format'); diff --git a/test/es-module/test-esm-resolve-type.js b/test/es-module/test-esm-resolve-type.js index 3cc484188ad513..ba4dea03c8ac48 100644 --- a/test/es-module/test-esm-resolve-type.js +++ b/test/es-module/test-esm-resolve-type.js @@ -35,10 +35,11 @@ try { * ensure that resolving by full path does not return the format * with the defaultResolver */ - [ [ '/es-modules/package-type-module/index.js', undefined ], - [ '/es-modules/package-type-commonjs/index.js', undefined ], - [ '/es-modules/package-without-type/index.js', undefined ], - [ '/es-modules/package-without-pjson/index.js', undefined ], + [ + [ '/es-modules/package-type-module/index.js', 'module' ], + [ '/es-modules/package-type-commonjs/index.js', 'commonjs' ], + [ '/es-modules/package-without-type/index.js', 'commonjs' ], + [ '/es-modules/package-without-pjson/index.js', 'commonjs' ], ].forEach((testVariant) => { const [ testScript, expectedType ] = testVariant; const resolvedPath = path.resolve(fixtures.path(testScript)); @@ -49,12 +50,14 @@ try { /** * create a test module and try to resolve it by module name. * check the result is as expected + * + * for test-module-ne: everything .js that is not 'module' is 'commonjs' */ [ [ 'test-module-mainjs', 'js', 'module', 'module'], [ 'test-module-mainmjs', 'mjs', 'module', 'module'], [ 'test-module-cjs', 'js', 'commonjs', 'commonjs'], - [ 'test-module-ne', 'js', undefined, undefined], + [ 'test-module-ne', 'js', undefined, 'commonjs'], ].forEach((testVariant) => { const [ moduleName, moduleExtenstion, @@ -98,86 +101,149 @@ try { } }; - // Create a dummy dual package - // - /** - * this creates following directory structure: - * - * ./node_modules: - * |-> my-dual-package - * |-> es - * |-> index.js - * |-> package.json [2] - * |-> lib - * |-> index.js - * |->package.json [1] - * - * [1] - main package.json of the package - * - it contains: - * - type: 'commonjs' - * - main: 'lib/mainfile.js' - * - conditional exports for 'require' (lib/index.js) and - * 'import' (es/index.js) - * [2] - package.json add-on for the import case - * - it only contains: - * - type: 'module' - * - * in case the package is consumed as an ESM by importing it: - * import * as my-package from 'my-dual-package' - * it will cause the resolve method to return: - * { - * url: '/node_modules/my-dual-package/es/index.js', - * format: 'module' - * } - * - * following testcase ensures that resolve works correctly in this case - * returning the information as specified above. Source for 'url' value - * is [1], source for 'format' value is [2] - */ + function testDualPackageWithJsMainScriptAndModuleType() { + // Create a dummy dual package + // + /** + * this creates the following directory structure: + * + * ./node_modules: + * |-> my-dual-package + * |-> es + * |-> index.js + * |-> package.json [2] + * |-> lib + * |-> index.js + * |->package.json [1] + * + * in case the package is imported: + * import * as my-package from 'my-dual-package' + * it will cause the resolve method to return: + * { + * url: '/node_modules/my-dual-package/es/index.js', + * format: 'module' + * } + * + * following testcase ensures that resolve works correctly in this case + * returning the information as specified above. Source for 'url' value + * is [1], source for 'format' value is [2] + */ + + const moduleName = 'my-dual-package'; - const moduleName = 'my-dual-package'; - - const mDir = rel(`node_modules/${moduleName}`); - const esSubDir = rel(`node_modules/${moduleName}/es`); - const cjsSubDir = rel(`node_modules/${moduleName}/lib`); - const pkg = rel(`node_modules/${moduleName}/package.json`); - const esmPkg = rel(`node_modules/${moduleName}/es/package.json`); - const esScript = rel(`node_modules/${moduleName}/es/index.js`); - const cjsScript = rel(`node_modules/${moduleName}/lib/index.js`); - - createDir(nmDir); - createDir(mDir); - createDir(esSubDir); - createDir(cjsSubDir); - - const mainPkgJsonContent = { - type: 'commonjs', - main: 'lib/index.js', - exports: { - '.': { - 'require': './lib/index.js', - 'import': './es/index.js' - }, - './package.json': './package.json', - } - }; - const esmPkgJsonContent = { - type: 'module' - }; + const mDir = rel(`node_modules/${moduleName}`); + const esSubDir = rel(`node_modules/${moduleName}/es`); + const cjsSubDir = rel(`node_modules/${moduleName}/lib`); + const pkg = rel(`node_modules/${moduleName}/package.json`); + const esmPkg = rel(`node_modules/${moduleName}/es/package.json`); + const esScript = rel(`node_modules/${moduleName}/es/index.js`); + const cjsScript = rel(`node_modules/${moduleName}/lib/index.js`); - fs.writeFileSync(pkg, JSON.stringify(mainPkgJsonContent)); - fs.writeFileSync(esmPkg, JSON.stringify(esmPkgJsonContent)); - fs.writeFileSync(esScript, - 'export function esm-resolve-tester() {return 42}'); - fs.writeFileSync(cjsScript, - `module.exports = { - esm-resolve-tester: () => {return 42}}` - ); + createDir(nmDir); + createDir(mDir); + createDir(esSubDir); + createDir(cjsSubDir); + + const mainPkgJsonContent = { + type: 'commonjs', + exports: { + '.': { + 'require': './lib/index.js', + 'import': './es/index.js', + 'default': './lib/index.js' + }, + './package.json': './package.json', + } + }; + const esmPkgJsonContent = { + type: 'module' + }; + + fs.writeFileSync(pkg, JSON.stringify(mainPkgJsonContent)); + fs.writeFileSync(esmPkg, JSON.stringify(esmPkgJsonContent)); + fs.writeFileSync( + esScript, + 'export function esm-resolve-tester() {return 42}' + ); + fs.writeFileSync( + cjsScript, + 'module.exports = {esm-resolve-tester: () => {return 42}}' + ); + + // test the resolve + const resolveResult = resolve(`${moduleName}`); + assert.strictEqual(resolveResult.format, 'module'); + assert.ok(resolveResult.url.includes('my-dual-package/es/index.js')); + } + + testDualPackageWithJsMainScriptAndModuleType(); + + // TestParameters are ModuleName, mainRequireScript, mainImportScript, + // mainPackageType, subdirPkgJsonType, expectedResolvedFormat, mainSuffix + [ + [ 'mjs-mod-mod', 'index.js', 'index.mjs', 'module', 'module', 'module'], + [ 'mjs-com-com', 'idx.js', 'idx.mjs', 'commonjs', 'commonjs', 'module'], + [ 'mjs-mod-com', 'index.js', 'imp.mjs', 'module', 'commonjs', 'module'], + [ 'cjs-mod-mod', 'index.cjs', 'imp.cjs', 'module', 'module', 'commonjs'], + [ 'js-com-com', 'index.js', 'imp.js', 'commonjs', 'commonjs', 'commonjs'], + [ 'js-com-mod', 'index.js', 'imp.js', 'commonjs', 'module', 'module'], + [ 'qmod', 'index.js', 'imp.js', 'commonjs', 'module', 'module', '?k=v'], + [ 'hmod', 'index.js', 'imp.js', 'commonjs', 'module', 'module', '#Key'], + [ 'qhmod', 'index.js', 'imp.js', 'commonjs', 'module', 'module', '?k=v#h'], + [ 'ts-mod-com', 'index.js', 'imp.ts', 'module', 'commonjs', undefined], + ].forEach((testVariant) => { + const [ + moduleName, + mainRequireScript, + mainImportScript, + mainPackageType, + subdirPackageType, + expectedResolvedFormat, + mainSuffix = '' ] = testVariant; + + const mDir = rel(`node_modules/${moduleName}`); + const subDir = rel(`node_modules/${moduleName}/subdir`); + const pkg = rel(`node_modules/${moduleName}/package.json`); + const subdirPkg = rel(`node_modules/${moduleName}/subdir/package.json`); + const esScript = rel(`node_modules/${moduleName}/subdir/${mainImportScript}`); + const cjsScript = rel(`node_modules/${moduleName}/subdir/${mainRequireScript}`); + + createDir(nmDir); + createDir(mDir); + createDir(subDir); + + const mainPkgJsonContent = { + type: mainPackageType, + exports: { + '.': { + 'require': `./subdir/${mainRequireScript}${mainSuffix}`, + 'import': `./subdir/${mainImportScript}${mainSuffix}`, + 'default': `./subdir/${mainRequireScript}${mainSuffix}` + }, + './package.json': './package.json', + } + }; + const subdirPkgJsonContent = { + type: `${subdirPackageType}` + }; + + fs.writeFileSync(pkg, JSON.stringify(mainPkgJsonContent)); + fs.writeFileSync(subdirPkg, JSON.stringify(subdirPkgJsonContent)); + fs.writeFileSync( + esScript, + 'export function esm-resolve-tester() {return 42}' + ); + fs.writeFileSync( + cjsScript, + 'module.exports = {esm-resolve-tester: () => {return 42}}' + ); + + // test the resolve + const resolveResult = resolve(`${moduleName}`); + assert.strictEqual(resolveResult.format, expectedResolvedFormat); + assert.ok(resolveResult.url.endsWith(`${moduleName}/subdir/${mainImportScript}${mainSuffix}`)); + }); - // test the resolve - const resolveResult = resolve(`${moduleName}`); - assert.strictEqual(resolveResult.format, 'module'); - assert.ok(resolveResult.url.includes('my-dual-package/es/index.js')); } finally { process.chdir(previousCwd); fs.rmSync(nmDir, { recursive: true, force: true });