From a149086b873ac4975f293dbbb0f9832f99bf0e10 Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Fri, 17 Dec 2021 12:44:49 +0100 Subject: [PATCH 01/14] 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 --- lib/internal/modules/esm/resolve.js | 7 +- test/es-module/test-esm-resolve-type.js | 240 ++++++++++++++++-------- 2 files changed, 169 insertions(+), 78 deletions(-) diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 9e05b1c9c97379..ce5b4a3948cfa5 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -467,7 +467,12 @@ function resolvePackageTargetString( const composeResult = (resolved) => { let format; try { - format = getPackageType(resolved); + // extension has higher priority than package.json type descriptor + if (StringPrototypeEndsWith(resolved.href, '.mjs')) { + format = 'module'; + } else { + format = getPackageType(resolved); + } } catch (err) { if (err.code === 'ERR_INVALID_FILE_URL_PATH') { const invalidModuleErr = new ERR_INVALID_MODULE_SPECIFIER( diff --git a/test/es-module/test-esm-resolve-type.js b/test/es-module/test-esm-resolve-type.js index 3cc484188ad513..870d7108c35461 100644 --- a/test/es-module/test-esm-resolve-type.js +++ b/test/es-module/test-esm-resolve-type.js @@ -98,86 +98,172 @@ 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 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] + */ - 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 moduleName = 'my-dual-package'; - 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}}` - ); + 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' + }; + + 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')); + } + + function testDualPackageWithMjsMainScriptAndCJSType() { + + // Additional test for following scenario + /** + * this creates following directory structure: + * + * ./node_modules: + * |-> dual-mjs-pjson + * |-> subdir + * |-> index.mjs [3] + * |-> package.json [2] + * |-> index.js + * |->package.json [1] + * + * [1] - main package.json of the package + * - it contains: + * - type: 'commonjs' + * - main: 'subdir/index.js' + * - conditional exports for 'require' (subdir/index.js) and + * 'import' (subdir/index.mjs) + * [2] - package.json add-on for the import case + * - it only contains: + * - type: 'commonjs' + * [3] - main script for the `import` case + * + * in case the package is consumed as an ESM by importing it: + * import * as my-package from 'dual-mjs-pjson' + * it will cause the resolve method to return: + * { + * url: '/node_modules/dual-mjs-pjson/subdir/index.mjs', + * 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 the file extension of [3] + */ + const moduleName = 'dual-mjs-pjson'; + + 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/index.mjs`); + const cjsScript = rel(`node_modules/${moduleName}/subdir/index.js`); + + createDir(nmDir); + createDir(mDir); + createDir(subDir); + + const mainPkgJsonContent = { + type: 'commonjs', + main: 'lib/index.js', + exports: { + '.': { + 'require': './subdir/index.js', + 'import': './subdir/index.mjs' + }, + './package.json': './package.json', + } + }; + const subdirPkgJsonContent = { + type: 'commonjs' + }; + + 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, 'module'); + assert.ok(resolveResult.url.includes(`${moduleName}/subdir/index.mjs`)); + } + + testDualPackageWithJsMainScriptAndModuleType(); + testDualPackageWithMjsMainScriptAndCJSType(); - // 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 }); From db7a12a8811b610619ca094af80d3e91d6cf12fc Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Fri, 17 Dec 2021 13:06:40 +0100 Subject: [PATCH 02/14] fixup! loader: fix package resolution for edge case --- lib/internal/modules/esm/resolve.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index ce5b4a3948cfa5..3cdffe3f77ed3e 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -467,7 +467,7 @@ function resolvePackageTargetString( const composeResult = (resolved) => { let format; try { - // extension has higher priority than package.json type descriptor + // Extension has higher priority than package.json type descriptor if (StringPrototypeEndsWith(resolved.href, '.mjs')) { format = 'module'; } else { From 6f7d8714b9376ec3c9eedebd4df71a62042d101a Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Fri, 17 Dec 2021 15:14:08 +0100 Subject: [PATCH 03/14] rewrite based on review comments and defaultGetFormat implementation --- lib/internal/modules/esm/resolve.js | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 3cdffe3f77ed3e..002661cf9b40ae 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -37,7 +37,7 @@ const { getOptionValue } = require('internal/options'); const policy = getOptionValue('--experimental-policy') ? require('internal/process/policy') : null; -const { sep, relative, resolve } = require('path'); +const { sep, relative, resolve, extname } = require('path'); const preserveSymlinks = getOptionValue('--preserve-symlinks'); const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); const typeFlag = getOptionValue('--input-type'); @@ -62,6 +62,9 @@ const userConditions = getOptionValue('--conditions'); const noAddons = getOptionValue('--no-addons'); const addonConditions = noAddons ? [] : ['node-addons']; +const experimentalSpecifierResolution = + getOptionValue('--experimental-specifier-resolution'); + const DEFAULT_CONDITIONS = ObjectFreeze([ 'node', 'import', @@ -467,11 +470,19 @@ function resolvePackageTargetString( const composeResult = (resolved) => { let format; try { - // Extension has higher priority than package.json type descriptor - if (StringPrototypeEndsWith(resolved.href, '.mjs')) { - format = 'module'; - } else { + const ext = extname(resolved.pathname); + if (ext === '.js') { format = getPackageType(resolved); + } else { + format = extensionFormatMap[ext]; + } + if (!format) { + if (experimentalSpecifierResolution === 'node') { + process.emitWarning( + 'The Node.js specifier resolution in ESM is experimental.', + 'ExperimentalWarning'); + format = legacyExtensionFormatMap[ext]; + } } } catch (err) { if (err.code === 'ERR_INVALID_FILE_URL_PATH') { @@ -1112,4 +1123,6 @@ module.exports = { }; // cycle -const { defaultGetFormat } = require('internal/modules/esm/get_format'); +const { defaultGetFormat, + extensionFormatMap, + legacyExtensionFormatMap } = require('internal/modules/esm/get_format'); From 435dc9bf9f9c7bd41728860dc1200c8f094fcd93 Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Fri, 17 Dec 2021 16:53:56 +0100 Subject: [PATCH 04/14] review comments, rework tests --- lib/internal/modules/esm/get_format.js | 12 +++- lib/internal/modules/esm/resolve.js | 51 +++++++++------- test/es-module/test-esm-resolve-type.js | 77 +++++++++---------------- 3 files changed, 65 insertions(+), 75 deletions(-) diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js index 7c07e5b1f7ce17..4621b3a6c40e75 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'; @@ -64,9 +66,12 @@ const protocolHandlers = ObjectAssign(ObjectCreate(null), { } if (!format) { if (experimentalSpecifierResolution === 'node') { - process.emitWarning( - 'The Node.js specifier resolution in ESM is experimental.', - 'ExperimentalWarning'); + if (!experimentalSpecifierResolutionWarned) { + process.emitWarning( + 'The Node.js specifier resolution in ESM is experimental.', + 'ExperimentalWarning'); + experimentalSpecifierResolutionWarned = true; + } format = legacyExtensionFormatMap[ext]; } else { throw new ERR_UNKNOWN_FILE_EXTENSION(ext, fileURLToPath(url)); @@ -90,4 +95,5 @@ module.exports = { defaultGetFormat, extensionFormatMap, legacyExtensionFormatMap, + experimentalSpecifierResolutionWarned }; diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 002661cf9b40ae..594b1c9ac2d9fb 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -469,30 +469,34 @@ function resolvePackageTargetString( const composeResult = (resolved) => { let format; - try { - const ext = extname(resolved.pathname); - if (ext === '.js') { + + const ext = extname(resolved.pathname); + if (ext === '.js') { + try { format = getPackageType(resolved); - } else { - format = extensionFormatMap[ext]; - } - if (!format) { - if (experimentalSpecifierResolution === 'node') { - process.emitWarning( - 'The Node.js specifier resolution in ESM is experimental.', - 'ExperimentalWarning'); - format = legacyExtensionFormatMap[ext]; + } 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; } - } 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; + } else { + format = extensionFormatMap[ext]; + } + + if (format == null && experimentalSpecifierResolution === 'node') { + if (!experimentalSpecifierResolutionWarned) { + process.emitWarning( + 'The Node.js specifier resolution in ESM is experimental.', + 'ExperimentalWarning'); + experimentalSpecifierResolutionWarned = true; } - throw err; + format = legacyExtensionFormatMap[ext]; } + return { resolved, ...(format !== 'none') && { format } }; }; @@ -1123,6 +1127,9 @@ module.exports = { }; // cycle -const { defaultGetFormat, - extensionFormatMap, - legacyExtensionFormatMap } = require('internal/modules/esm/get_format'); +let { + defaultGetFormat, + extensionFormatMap, + legacyExtensionFormatMap, + experimentalSpecifierResolutionWarned, +} = 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 870d7108c35461..dd3c20eed64c2d 100644 --- a/test/es-module/test-esm-resolve-type.js +++ b/test/es-module/test-esm-resolve-type.js @@ -181,69 +181,49 @@ try { assert.ok(resolveResult.url.includes('my-dual-package/es/index.js')); } - function testDualPackageWithMjsMainScriptAndCJSType() { + testDualPackageWithJsMainScriptAndModuleType(); - // Additional test for following scenario - /** - * this creates following directory structure: - * - * ./node_modules: - * |-> dual-mjs-pjson - * |-> subdir - * |-> index.mjs [3] - * |-> package.json [2] - * |-> index.js - * |->package.json [1] - * - * [1] - main package.json of the package - * - it contains: - * - type: 'commonjs' - * - main: 'subdir/index.js' - * - conditional exports for 'require' (subdir/index.js) and - * 'import' (subdir/index.mjs) - * [2] - package.json add-on for the import case - * - it only contains: - * - type: 'commonjs' - * [3] - main script for the `import` case - * - * in case the package is consumed as an ESM by importing it: - * import * as my-package from 'dual-mjs-pjson' - * it will cause the resolve method to return: - * { - * url: '/node_modules/dual-mjs-pjson/subdir/index.mjs', - * 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 the file extension of [3] - */ - const moduleName = 'dual-mjs-pjson'; + // TestParameters are ModuleName, mainRequireScript, mainImportScript, + // mainPackageType, subdirPkgJsonType, expectedResolvedFormat + [ [ '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'], + [ 'js-com-com', 'index.js', 'imp.js', 'commonjs', 'commonjs', 'commonjs'], + [ 'js-com-mod', 'index.js', 'imp.js', 'commonjs', 'module', 'module'], + [ 'ts-mod-com', 'index.js', 'imp.ts', 'module', 'commonjs', undefined], + ].forEach((testVariant) => { + const [ + moduleName, + mainRequireScript, + mainImportScript, + mainPackageType, + subdirPackageType, + expectedResolvedFormat ] = 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/index.mjs`); - const cjsScript = rel(`node_modules/${moduleName}/subdir/index.js`); + 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: 'commonjs', - main: 'lib/index.js', + type: mainPackageType, + main: `./subdir/${mainRequireScript}`, exports: { '.': { - 'require': './subdir/index.js', - 'import': './subdir/index.mjs' + 'require': `./subdir/${mainRequireScript}`, + 'import': `./subdir/${mainImportScript}` }, './package.json': './package.json', } }; const subdirPkgJsonContent = { - type: 'commonjs' + type: `${subdirPackageType}` }; fs.writeFileSync(pkg, JSON.stringify(mainPkgJsonContent)); @@ -257,12 +237,9 @@ try { // test the resolve const resolveResult = resolve(`${moduleName}`); - assert.strictEqual(resolveResult.format, 'module'); - assert.ok(resolveResult.url.includes(`${moduleName}/subdir/index.mjs`)); - } - - testDualPackageWithJsMainScriptAndModuleType(); - testDualPackageWithMjsMainScriptAndCJSType(); + assert.strictEqual(resolveResult.format, expectedResolvedFormat); + assert.ok(resolveResult.url.includes(`${moduleName}/subdir/${mainImportScript}`)); + }); } finally { process.chdir(previousCwd); From f9fe067bbd2eceb0661090a99e411d8d5c0ea73b Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Fri, 17 Dec 2021 19:58:48 +0100 Subject: [PATCH 05/14] review comments, test cases added for query and hash export urls --- lib/internal/modules/esm/get_format.js | 37 +++++++++++++++---------- lib/internal/modules/esm/resolve.js | 20 ++----------- test/es-module/test-esm-resolve-type.js | 11 ++++++-- 3 files changed, 33 insertions(+), 35 deletions(-) diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js index 4621b3a6c40e75..131c4b8049fb07 100644 --- a/lib/internal/modules/esm/get_format.js +++ b/lib/internal/modules/esm/get_format.js @@ -62,20 +62,8 @@ const protocolHandlers = ObjectAssign(ObjectCreate(null), { if (ext === '.js') { format = getPackageType(parsed.href) === 'module' ? 'module' : 'commonjs'; } else { - format = extensionFormatMap[ext]; - } - if (!format) { - if (experimentalSpecifierResolution === 'node') { - if (!experimentalSpecifierResolutionWarned) { - process.emitWarning( - 'The Node.js specifier resolution in ESM is experimental.', - 'ExperimentalWarning'); - experimentalSpecifierResolutionWarned = true; - } - format = legacyExtensionFormatMap[ext]; - } else { - throw new ERR_UNKNOWN_FILE_EXTENSION(ext, fileURLToPath(url)); - } + format = extensionFormatMap[ext] ?? + throwIfNotExperimentalSpecifierResolution(ext, url); } return format || null; @@ -83,6 +71,25 @@ const protocolHandlers = ObjectAssign(ObjectCreate(null), { 'node:'() { return 'builtin'; }, }); +function throwIfNotExperimentalSpecifierResolution(ext, url) { + if (experimentalSpecifierResolution !== 'node') { + throw new ERR_UNKNOWN_FILE_EXTENSION(ext, fileURLToPath(url)); + } + return getLegacyExtensionFormat(ext); +} + +function getLegacyExtensionFormat(ext) { + if (experimentalSpecifierResolution === 'node') { + if (!experimentalSpecifierResolutionWarned) { + process.emitWarning( + 'The Node.js specifier resolution in ESM is experimental.', + 'ExperimentalWarning'); + experimentalSpecifierResolutionWarned = true; + } + } + return legacyExtensionFormatMap[ext]; +} + function defaultGetFormat(url, context) { const parsed = new URL(url); @@ -95,5 +102,5 @@ module.exports = { defaultGetFormat, extensionFormatMap, legacyExtensionFormatMap, - experimentalSpecifierResolutionWarned + getLegacyExtensionFormat }; diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 594b1c9ac2d9fb..21f3c4703594c9 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -62,9 +62,6 @@ const userConditions = getOptionValue('--conditions'); const noAddons = getOptionValue('--no-addons'); const addonConditions = noAddons ? [] : ['node-addons']; -const experimentalSpecifierResolution = - getOptionValue('--experimental-specifier-resolution'); - const DEFAULT_CONDITIONS = ObjectFreeze([ 'node', 'import', @@ -484,17 +481,7 @@ function resolvePackageTargetString( throw err; } } else { - format = extensionFormatMap[ext]; - } - - if (format == null && experimentalSpecifierResolution === 'node') { - if (!experimentalSpecifierResolutionWarned) { - process.emitWarning( - 'The Node.js specifier resolution in ESM is experimental.', - 'ExperimentalWarning'); - experimentalSpecifierResolutionWarned = true; - } - format = legacyExtensionFormatMap[ext]; + format = extensionFormatMap[ext] ?? getLegacyExtensionFormat(ext); } return { resolved, ...(format !== 'none') && { format } }; @@ -1127,9 +1114,8 @@ module.exports = { }; // cycle -let { +const { defaultGetFormat, extensionFormatMap, - legacyExtensionFormatMap, - experimentalSpecifierResolutionWarned, + getLegacyExtensionFormat, } = 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 dd3c20eed64c2d..209fb5d291c22c 100644 --- a/test/es-module/test-esm-resolve-type.js +++ b/test/es-module/test-esm-resolve-type.js @@ -184,12 +184,15 @@ try { testDualPackageWithJsMainScriptAndModuleType(); // TestParameters are ModuleName, mainRequireScript, mainImportScript, - // mainPackageType, subdirPkgJsonType, expectedResolvedFormat + // 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'], [ '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 [ @@ -198,7 +201,8 @@ try { mainImportScript, mainPackageType, subdirPackageType, - expectedResolvedFormat ] = testVariant; + expectedResolvedFormat, + mainSuffix ] = testVariant; const mDir = rel(`node_modules/${moduleName}`); const subDir = rel(`node_modules/${moduleName}/subdir`); @@ -211,13 +215,14 @@ try { createDir(mDir); createDir(subDir); + const mainScript = mainImportScript + (mainSuffix ?? ''); const mainPkgJsonContent = { type: mainPackageType, main: `./subdir/${mainRequireScript}`, exports: { '.': { 'require': `./subdir/${mainRequireScript}`, - 'import': `./subdir/${mainImportScript}` + 'import': `./subdir/${mainScript}` }, './package.json': './package.json', } From 0bb04f24c0c3124341368cbfebee3cb2a25ef4b5 Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Fri, 17 Dec 2021 20:11:02 +0100 Subject: [PATCH 06/14] review round --- lib/internal/modules/esm/resolve.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 21f3c4703594c9..21f16557d19438 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -470,7 +470,7 @@ function resolvePackageTargetString( const ext = extname(resolved.pathname); if (ext === '.js') { try { - format = getPackageType(resolved); + format = getPackageType(resolved) === 'module' ? 'module' : 'commonjs'; } catch (err) { if (err.code === 'ERR_INVALID_FILE_URL_PATH') { const invalidModuleErr = new ERR_INVALID_MODULE_SPECIFIER( From a72ad73d546ec93f4d3a21383ccb4f64d0a71e37 Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Sat, 18 Dec 2021 16:41:02 +0100 Subject: [PATCH 07/14] refactoring amendFormatToUrl out in moduleResolve + other review comments --- lib/internal/modules/cjs/loader.js | 4 +- lib/internal/modules/esm/get_format.js | 15 ++-- lib/internal/modules/esm/resolve.js | 99 ++++++++++++++----------- test/es-module/test-esm-resolve-type.js | 54 +++++++------- 4 files changed, 90 insertions(+), 82 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 131c4b8049fb07..eb651b747c386d 100644 --- a/lib/internal/modules/esm/get_format.js +++ b/lib/internal/modules/esm/get_format.js @@ -79,13 +79,14 @@ function throwIfNotExperimentalSpecifierResolution(ext, url) { } function getLegacyExtensionFormat(ext) { - if (experimentalSpecifierResolution === 'node') { - if (!experimentalSpecifierResolutionWarned) { - process.emitWarning( - 'The Node.js specifier resolution in ESM is experimental.', - 'ExperimentalWarning'); - experimentalSpecifierResolutionWarned = true; - } + if ( + experimentalSpecifierResolution === 'node' && + !experimentalSpecifierResolutionWarned + ) { + process.emitWarning( + 'The Node.js specifier resolution in ESM is experimental.', + 'ExperimentalWarning'); + experimentalSpecifierResolutionWarned = true; } return legacyExtensionFormatMap[ext]; } diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 21f16557d19438..1e4e333439b88c 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -464,29 +464,6 @@ const patternRegEx = /\*/g; function resolvePackageTargetString( target, subpath, match, packageJSONUrl, base, pattern, internal, conditions) { - const composeResult = (resolved) => { - let format; - - const ext = extname(resolved.pathname); - if (ext === '.js') { - try { - format = getPackageType(resolved) === 'module' ? 'module' : 'commonjs'; - } 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; - } - } else { - format = extensionFormatMap[ext] ?? getLegacyExtensionFormat(ext); - } - - return { resolved, ...(format !== 'none') && { format } }; - }; - if (subpath !== '' && !pattern && target[target.length - 1] !== '/') throwInvalidPackageTarget(match, target, packageJSONUrl, internal, base); @@ -519,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 ? @@ -528,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); } /** @@ -550,7 +531,13 @@ function resolvePackageTarget(packageJSONUrl, target, subpath, packageSubpath, base, pattern, internal, conditions) { if (typeof target === 'string') { return resolvePackageTargetString( - target, subpath, packageSubpath, packageJSONUrl, base, pattern, internal, + target, + subpath, + packageSubpath, + packageJSONUrl, + base, + pattern, + internal, conditions); } else if (ArrayIsArray(target)) { if (target.length === 0) @@ -760,7 +747,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 = ''; @@ -792,7 +779,7 @@ function packageImportsResolve(name, base, conditions) { bestMatch, base, true, true, conditions); if (resolveResult != null) { - return resolveResult.resolved; + return resolveResult; } } } @@ -856,7 +843,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); @@ -895,19 +882,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); @@ -952,6 +934,29 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks) { // Ok since relative URLs cannot parse as URLs. let resolved; let format; + const amendFormatToUrl = (resolved) => { + let format; + + const ext = extname(resolved.pathname); + if (ext === '.js') { + try { + format = getPackageType(resolved) === 'module' ? 'module' : 'commonjs'; + } 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; + } + } else { + format = extensionFormatMap[ext] ?? getLegacyExtensionFormat(ext); + } + + return { resolved, ...(format !== 'none') && { format } }; + }; + if (shouldBeTreatedAsRelativeOrAbsolutePath(specifier)) { resolved = new URL(specifier, base); } else if (specifier[0] === '#') { @@ -960,7 +965,13 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks) { try { resolved = new URL(specifier); } catch { - ({ resolved, format } = packageResolve(specifier, base, conditions)); + ({ resolved, format } = + amendFormatToUrl( + packageResolve( + specifier, + base, + conditions + ))); } } if (resolved.protocol !== 'file:') { diff --git a/test/es-module/test-esm-resolve-type.js b/test/es-module/test-esm-resolve-type.js index 209fb5d291c22c..9c904849989f80 100644 --- a/test/es-module/test-esm-resolve-type.js +++ b/test/es-module/test-esm-resolve-type.js @@ -49,12 +49,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, @@ -102,7 +104,7 @@ try { // Create a dummy dual package // /** - * this creates following directory structure: + * this creates the following directory structure: * * ./node_modules: * |-> my-dual-package @@ -113,17 +115,7 @@ try { * |-> 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: + * in case the package is imported: * import * as my-package from 'my-dual-package' * it will cause the resolve method to return: * { @@ -168,11 +160,13 @@ try { 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}}` + fs.writeFileSync( + esScript, + 'export function esm-resolve-tester() {return 42}' + ); + fs.writeFileSync( + cjsScript, + 'module.exports = {esm-resolve-tester: () => {return 42}}' ); // test the resolve @@ -185,7 +179,8 @@ try { // TestParameters are ModuleName, mainRequireScript, mainImportScript, // mainPackageType, subdirPkgJsonType, expectedResolvedFormat, mainSuffix - [ [ 'mjs-mod-mod', 'index.js', 'index.mjs', 'module', 'module', 'module'], + [ + [ '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'], [ 'js-com-com', 'index.js', 'imp.js', 'commonjs', 'commonjs', 'commonjs'], @@ -202,7 +197,7 @@ try { mainPackageType, subdirPackageType, expectedResolvedFormat, - mainSuffix ] = testVariant; + mainSuffix = '' ] = testVariant; const mDir = rel(`node_modules/${moduleName}`); const subDir = rel(`node_modules/${moduleName}/subdir`); @@ -215,14 +210,13 @@ try { createDir(mDir); createDir(subDir); - const mainScript = mainImportScript + (mainSuffix ?? ''); const mainPkgJsonContent = { type: mainPackageType, main: `./subdir/${mainRequireScript}`, exports: { '.': { - 'require': `./subdir/${mainRequireScript}`, - 'import': `./subdir/${mainScript}` + 'require': `./subdir/${mainRequireScript}${mainSuffix}`, + 'import': `./subdir/${mainImportScript}${mainSuffix}` }, './package.json': './package.json', } @@ -233,17 +227,19 @@ try { 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}}` + 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.includes(`${moduleName}/subdir/${mainImportScript}`)); + assert.ok(resolveResult.url.endsWith(`${moduleName}/subdir/${mainImportScript}${mainSuffix}`)); }); } finally { From 05d7f87d0ebd6653af85b00928256042b75c2d85 Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Sun, 19 Dec 2021 09:35:43 +0100 Subject: [PATCH 08/14] refactor to getPackageFormat --- lib/internal/modules/esm/get_format.js | 31 ++++++++++++++---------- lib/internal/modules/esm/resolve.js | 33 +++++++------------------- 2 files changed, 27 insertions(+), 37 deletions(-) diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js index eb651b747c386d..79e1bf269f9ffd 100644 --- a/lib/internal/modules/esm/get_format.js +++ b/lib/internal/modules/esm/get_format.js @@ -56,17 +56,7 @@ 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] ?? - throwIfNotExperimentalSpecifierResolution(ext, url); - } - - return format || null; + return getPackageFormat(parsed, true, url) || null; }, 'node:'() { return 'builtin'; }, }); @@ -91,6 +81,23 @@ function getLegacyExtensionFormat(ext) { return legacyExtensionFormatMap[ext]; } +function getPackageFormat(url, throwIfNotResolved, urlForThrow) { + let format; + + const ext = extname(url.pathname); + if (ext === '.js') { + format = getPackageType(url) === 'module' ? 'module' : 'commonjs'; + } else { + format = extensionFormatMap[ext] ?? + (throwIfNotResolved ? + throwIfNotExperimentalSpecifierResolution(ext, urlForThrow) : + getLegacyExtensionFormat(ext) + ); + } + + return format; +} + function defaultGetFormat(url, context) { const parsed = new URL(url); @@ -103,5 +110,5 @@ module.exports = { defaultGetFormat, extensionFormatMap, legacyExtensionFormatMap, - getLegacyExtensionFormat + getPackageFormat }; diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 1e4e333439b88c..90fefeb3a007a8 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -37,7 +37,7 @@ const { getOptionValue } = require('internal/options'); const policy = getOptionValue('--experimental-policy') ? require('internal/process/policy') : null; -const { sep, relative, resolve, extname } = require('path'); +const { sep, relative, resolve } = require('path'); const preserveSymlinks = getOptionValue('--preserve-symlinks'); const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); const typeFlag = getOptionValue('--input-type'); @@ -922,6 +922,12 @@ function shouldBeTreatedAsRelativeOrAbsolutePath(specifier) { return isRelativeSpecifier(specifier); } +function amendFormatToUrl(resolved) { + const format = getPackageFormat(resolved, false); + + return { resolved, ...(format !== null) && { format } }; +} + /** * @param {string} specifier * @param {string | URL | undefined} base @@ -934,28 +940,6 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks) { // Ok since relative URLs cannot parse as URLs. let resolved; let format; - const amendFormatToUrl = (resolved) => { - let format; - - const ext = extname(resolved.pathname); - if (ext === '.js') { - try { - format = getPackageType(resolved) === 'module' ? 'module' : 'commonjs'; - } 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; - } - } else { - format = extensionFormatMap[ext] ?? getLegacyExtensionFormat(ext); - } - - return { resolved, ...(format !== 'none') && { format } }; - }; if (shouldBeTreatedAsRelativeOrAbsolutePath(specifier)) { resolved = new URL(specifier, base); @@ -1127,6 +1111,5 @@ module.exports = { // cycle const { defaultGetFormat, - extensionFormatMap, - getLegacyExtensionFormat, + getPackageFormat, } = require('internal/modules/esm/get_format'); From 16f422f5dc29f4c9ce8a4ce6ee49cf3a2f818dc3 Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Sun, 19 Dec 2021 13:50:45 +0100 Subject: [PATCH 09/14] next review round --- lib/internal/modules/esm/get_format.js | 6 +++--- lib/internal/modules/esm/resolve.js | 4 ++-- test/es-module/test-esm-resolve-type.js | 8 ++++---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js index 79e1bf269f9ffd..ef8e03e247177f 100644 --- a/lib/internal/modules/esm/get_format.js +++ b/lib/internal/modules/esm/get_format.js @@ -56,7 +56,7 @@ const protocolHandlers = ObjectAssign(ObjectCreate(null), { return format; }, 'file:'(parsed, url) { - return getPackageFormat(parsed, true, url) || null; + return getModuleFileFormat(parsed, true, url) || null; }, 'node:'() { return 'builtin'; }, }); @@ -81,7 +81,7 @@ function getLegacyExtensionFormat(ext) { return legacyExtensionFormatMap[ext]; } -function getPackageFormat(url, throwIfNotResolved, urlForThrow) { +function getModuleFileFormat(url, throwIfNotResolved, urlForThrow) { let format; const ext = extname(url.pathname); @@ -110,5 +110,5 @@ module.exports = { defaultGetFormat, extensionFormatMap, legacyExtensionFormatMap, - getPackageFormat + getModuleFileFormat }; diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 90fefeb3a007a8..ce2ac8361244c9 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -923,7 +923,7 @@ function shouldBeTreatedAsRelativeOrAbsolutePath(specifier) { } function amendFormatToUrl(resolved) { - const format = getPackageFormat(resolved, false); + const format = getModuleFileFormat(resolved, false); return { resolved, ...(format !== null) && { format } }; } @@ -1111,5 +1111,5 @@ module.exports = { // cycle const { defaultGetFormat, - getPackageFormat, + getModuleFileFormat, } = 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 9c904849989f80..432f4a95fc964f 100644 --- a/test/es-module/test-esm-resolve-type.js +++ b/test/es-module/test-esm-resolve-type.js @@ -145,11 +145,11 @@ try { const mainPkgJsonContent = { type: 'commonjs', - main: 'lib/index.js', exports: { '.': { 'require': './lib/index.js', - 'import': './es/index.js' + 'import': './es/index.js', + 'default': './lib/index.js' }, './package.json': './package.json', } @@ -212,11 +212,11 @@ try { const mainPkgJsonContent = { type: mainPackageType, - main: `./subdir/${mainRequireScript}`, exports: { '.': { 'require': `./subdir/${mainRequireScript}${mainSuffix}`, - 'import': `./subdir/${mainImportScript}${mainSuffix}` + 'import': `./subdir/${mainImportScript}${mainSuffix}`, + 'default': `./subdir/${mainRequireScript}${mainSuffix}` }, './package.json': './package.json', } From a4c345658c63d1d0b0bb3c4b196b5fa0235da2fe Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Mon, 20 Dec 2021 07:55:15 +0100 Subject: [PATCH 10/14] review findings --- lib/internal/modules/esm/get_format.js | 2 +- lib/internal/modules/esm/resolve.js | 8 +------- test/es-module/test-esm-resolve-type.js | 1 + 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js index ef8e03e247177f..d15bd73e402a78 100644 --- a/lib/internal/modules/esm/get_format.js +++ b/lib/internal/modules/esm/get_format.js @@ -109,6 +109,6 @@ function defaultGetFormat(url, context) { module.exports = { defaultGetFormat, extensionFormatMap, + getModuleFileFormat, legacyExtensionFormatMap, - getModuleFileFormat }; diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index ce2ac8361244c9..b6b835646fbcbb 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -531,13 +531,7 @@ function resolvePackageTarget(packageJSONUrl, target, subpath, packageSubpath, base, pattern, internal, conditions) { if (typeof target === 'string') { return resolvePackageTargetString( - target, - subpath, - packageSubpath, - packageJSONUrl, - base, - pattern, - internal, + target, subpath, packageSubpath, packageJSONUrl, base, pattern, internal, conditions); } else if (ArrayIsArray(target)) { if (target.length === 0) diff --git a/test/es-module/test-esm-resolve-type.js b/test/es-module/test-esm-resolve-type.js index 432f4a95fc964f..486070c401562e 100644 --- a/test/es-module/test-esm-resolve-type.js +++ b/test/es-module/test-esm-resolve-type.js @@ -183,6 +183,7 @@ try { [ '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'], From 9a45c2b03fc4deeec8331b84b4b9a12b94e28b85 Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Tue, 21 Dec 2021 09:16:38 +0100 Subject: [PATCH 11/14] amendFormatToUrl also if resolve by relative path to module --- lib/internal/modules/esm/resolve.js | 19 ++++++++++++++----- test/es-module/test-esm-resolve-type.js | 9 +++++---- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index b6b835646fbcbb..aed5bea96932e3 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -941,8 +941,12 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks) { resolved = packageImportsResolve(specifier, base, conditions); } else { try { - resolved = new URL(specifier); - } catch { + ({ resolved, format } = + amendFormatToUrl(new URL(specifier))); + } catch (err) { + if (err.code === 'ERR_INVALID_URL_SCHEME') { + throwIfUnsupportedURLProtocol(new URL(specifier)); + } ({ resolved, format } = amendFormatToUrl( packageResolve( @@ -1010,6 +1014,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) { @@ -1082,9 +1093,7 @@ 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}`, diff --git a/test/es-module/test-esm-resolve-type.js b/test/es-module/test-esm-resolve-type.js index 486070c401562e..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)); From 8b741f0af088264a372bd84b92ec42be24c19fa7 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Tue, 21 Dec 2021 14:47:50 -0800 Subject: [PATCH 12/14] fixup: refactoring Signed-off-by: Gabriel Bota --- lib/internal/modules/esm/get_format.js | 43 ++++++++++---------- lib/internal/modules/esm/load.js | 2 +- lib/internal/modules/esm/resolve.js | 56 +++++++------------------- 3 files changed, 37 insertions(+), 64 deletions(-) diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js index d15bd73e402a78..2ce4056024eb19 100644 --- a/lib/internal/modules/esm/get_format.js +++ b/lib/internal/modules/esm/get_format.js @@ -55,19 +55,12 @@ const protocolHandlers = ObjectAssign(ObjectCreate(null), { return format; }, - 'file:'(parsed, url) { - return getModuleFileFormat(parsed, true, url) || null; + 'file:'(parsed, ignoreErrors) { + return getFileProtocolModuleFormat(parsed, ignoreErrors); }, 'node:'() { return 'builtin'; }, }); -function throwIfNotExperimentalSpecifierResolution(ext, url) { - if (experimentalSpecifierResolution !== 'node') { - throw new ERR_UNKNOWN_FILE_EXTENSION(ext, fileURLToPath(url)); - } - return getLegacyExtensionFormat(ext); -} - function getLegacyExtensionFormat(ext) { if ( experimentalSpecifierResolution === 'node' && @@ -81,34 +74,40 @@ function getLegacyExtensionFormat(ext) { return legacyExtensionFormatMap[ext]; } -function getModuleFileFormat(url, throwIfNotResolved, urlForThrow) { - let format; - +function getFileProtocolModuleFormat(url, ignoreErrors) { const ext = extname(url.pathname); if (ext === '.js') { - format = getPackageType(url) === 'module' ? 'module' : 'commonjs'; + return getPackageType(url) === 'module' ? 'module' : 'commonjs'; } else { - format = extensionFormatMap[ext] ?? - (throwIfNotResolved ? - throwIfNotExperimentalSpecifierResolution(ext, urlForThrow) : - getLegacyExtensionFormat(ext) - ); + 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; } +} - return format; +function defaultGetFormatWithoutErrors(url, context) { + const parsed = new URL(url); + if (!ObjectPrototypeHasOwnProperty(protocolHandlers, parsed.protocol)) + return null; + return protocolHandlers[parsed.protocol](parsed, url, 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, - getModuleFileFormat, legacyExtensionFormatMap, }; diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index 67123792e8903a..89151db2a04621 100644 --- a/lib/internal/modules/esm/load.js +++ b/lib/internal/modules/esm/load.js @@ -18,7 +18,7 @@ async function defaultLoad(url, context) { } = context; const { importAssertions } = context; - if (!format || !translators.has(format)) { + if (format === undefined) { format = defaultGetFormat(url); } diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index aed5bea96932e3..d30011c7c3884a 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); @@ -916,12 +916,6 @@ function shouldBeTreatedAsRelativeOrAbsolutePath(specifier) { return isRelativeSpecifier(specifier); } -function amendFormatToUrl(resolved) { - const format = getModuleFileFormat(resolved, false); - - return { resolved, ...(format !== null) && { format } }; -} - /** * @param {string} specifier * @param {string | URL | undefined} base @@ -933,39 +927,22 @@ 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] === '#') { resolved = packageImportsResolve(specifier, base, conditions); } else { try { - ({ resolved, format } = - amendFormatToUrl(new URL(specifier))); - } catch (err) { - if (err.code === 'ERR_INVALID_URL_SCHEME') { - throwIfUnsupportedURLProtocol(new URL(specifier)); - } - ({ resolved, format } = - amendFormatToUrl( - packageResolve( - specifier, - base, - conditions - ))); + resolved = new URL(specifier); + } + catch { + 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); } /** @@ -1061,15 +1038,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 @@ -1097,7 +1072,7 @@ function defaultResolve(specifier, context = {}, defaultResolveUnused) { return { url: `${url}`, - ...(format != null) && { format } + format: defaultGetFormatWithoutErrors(url) }; } @@ -1113,6 +1088,5 @@ module.exports = { // cycle const { - defaultGetFormat, - getModuleFileFormat, + defaultGetFormatWithoutErrors, } = require('internal/modules/esm/get_format'); From 01a3abc63e4aac778383d0a93dfeeca317dcd9e1 Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Wed, 22 Dec 2021 08:42:25 +0100 Subject: [PATCH 13/14] fixup: linter errors --- lib/internal/modules/esm/get_format.js | 20 ++++++++++---------- lib/internal/modules/esm/load.js | 1 - lib/internal/modules/esm/resolve.js | 3 +-- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js index 2ce4056024eb19..29de4c55d47c7a 100644 --- a/lib/internal/modules/esm/get_format.js +++ b/lib/internal/modules/esm/get_format.js @@ -78,17 +78,17 @@ function getFileProtocolModuleFormat(url, ignoreErrors) { const ext = extname(url.pathname); if (ext === '.js') { return getPackageType(url) === 'module' ? 'module' : 'commonjs'; - } else { - 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; } + + 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) { diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index 89151db2a04621..ce1f29357bf231 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'); /** diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index d30011c7c3884a..84ca303205226b 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -934,8 +934,7 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks) { } else { try { resolved = new URL(specifier); - } - catch { + } catch { resolved = packageResolve(specifier, base, conditions); } } From fdf0d4dfe8783b9af049ad018fc133a3a0be0d0d Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Wed, 22 Dec 2021 11:10:42 +0100 Subject: [PATCH 14/14] fixup: review findings --- lib/internal/modules/esm/get_format.js | 6 ++---- lib/internal/modules/esm/load.js | 2 +- lib/internal/modules/esm/resolve.js | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js index 29de4c55d47c7a..175988b17446d8 100644 --- a/lib/internal/modules/esm/get_format.js +++ b/lib/internal/modules/esm/get_format.js @@ -55,9 +55,7 @@ const protocolHandlers = ObjectAssign(ObjectCreate(null), { return format; }, - 'file:'(parsed, ignoreErrors) { - return getFileProtocolModuleFormat(parsed, ignoreErrors); - }, + 'file:': getFileProtocolModuleFormat, 'node:'() { return 'builtin'; }, }); @@ -95,7 +93,7 @@ function defaultGetFormatWithoutErrors(url, context) { const parsed = new URL(url); if (!ObjectPrototypeHasOwnProperty(protocolHandlers, parsed.protocol)) return null; - return protocolHandlers[parsed.protocol](parsed, url, true); + return protocolHandlers[parsed.protocol](parsed, true); } function defaultGetFormat(url, context) { diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index ce1f29357bf231..86fe0a77406ecf 100644 --- a/lib/internal/modules/esm/load.js +++ b/lib/internal/modules/esm/load.js @@ -17,7 +17,7 @@ async function defaultLoad(url, context) { } = context; const { importAssertions } = context; - if (format === undefined) { + if (format == null) { format = defaultGetFormat(url); } diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 84ca303205226b..4a1c487167c549 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -1071,7 +1071,7 @@ function defaultResolve(specifier, context = {}, defaultResolveUnused) { return { url: `${url}`, - format: defaultGetFormatWithoutErrors(url) + format: defaultGetFormatWithoutErrors(url), }; }