From cbd41acb3dd1a47205a8c4d5dbb91e717ac1340d Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Fri, 17 Dec 2021 12:44:49 +0100 Subject: [PATCH] loaders: 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 });