From c75d9535e4b43552d3ce216e2979da06c9d46ea1 Mon Sep 17 00:00:00 2001 From: Gabriel Bota <94833492+dygabo@users.noreply.github.com> Date: Mon, 13 Dec 2021 21:44:00 +0100 Subject: [PATCH] loader: return package format from defaultResolve if known MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a proposed modification of defaultResolve to return the package format in case it has been found during package resolution. The format will be returned as described in the documentation: https://nodejs.org/api/esm.html#resolvespecifier-context-defaultresolve There is one new unit test as well: test/es-module/test-esm-resolve-type.js PR-URL: https://github.com/nodejs/node/pull/40980 Reviewed-By: Bradley Farias Reviewed-By: Gerhard Stöbich Reviewed-By: Antoine du Hamel --- lib/internal/modules/esm/resolve.js | 154 ++++++++++----- .../test-esm-loader-resolve-type.mjs | 41 ++++ test/es-module/test-esm-resolve-type.js | 184 ++++++++++++++++++ .../es-module-loaders/hook-resolve-type.mjs | 21 ++ .../module-counter-by-type/index.js | 3 + .../module-counter-by-type/package.json | 4 + .../es-modules/package-without-pjson/index.js | 7 + 7 files changed, 368 insertions(+), 46 deletions(-) create mode 100644 test/es-module/test-esm-loader-resolve-type.mjs create mode 100644 test/es-module/test-esm-resolve-type.js create mode 100644 test/fixtures/es-module-loaders/hook-resolve-type.mjs create mode 100644 test/fixtures/es-modules/module-counter-by-type/index.js create mode 100644 test/fixtures/es-modules/module-counter-by-type/package.json create mode 100644 test/fixtures/es-modules/package-without-pjson/index.js diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index cef09f5ed100c3..1c007efc96f593 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -473,6 +473,23 @@ 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); @@ -488,7 +505,8 @@ function resolvePackageTargetString( const exportTarget = pattern ? RegExpPrototypeSymbolReplace(patternRegEx, target, () => subpath) : target + subpath; - return packageResolve(exportTarget, packageJSONUrl, conditions); + return packageResolve( + exportTarget, packageJSONUrl, conditions); } } throwInvalidPackageTarget(match, target, packageJSONUrl, internal, base); @@ -504,15 +522,18 @@ function resolvePackageTargetString( if (!StringPrototypeStartsWith(resolvedPath, packagePath)) throwInvalidPackageTarget(match, target, packageJSONUrl, internal, base); - if (subpath === '') return resolved; + if (subpath === '') return composeResult(resolved); if (RegExpPrototypeTest(invalidSegmentRegEx, subpath)) throwInvalidSubpath(match + subpath, packageJSONUrl, internal, base); - if (pattern) - return new URL(RegExpPrototypeSymbolReplace(patternRegEx, resolved.href, - () => subpath)); - return new URL(subpath, resolved); + if (pattern) { + return composeResult(new URL(RegExpPrototypeSymbolReplace(patternRegEx, + resolved.href, + () => subpath))); + } + + return composeResult(new URL(subpath, resolved)); } /** @@ -538,9 +559,9 @@ function resolvePackageTarget(packageJSONUrl, target, subpath, packageSubpath, let lastException; for (let i = 0; i < target.length; i++) { const targetItem = target[i]; - let resolved; + let resolveResult; try { - resolved = resolvePackageTarget( + resolveResult = resolvePackageTarget( packageJSONUrl, targetItem, subpath, packageSubpath, base, pattern, internal, conditions); } catch (e) { @@ -549,13 +570,13 @@ function resolvePackageTarget(packageJSONUrl, target, subpath, packageSubpath, continue; throw e; } - if (resolved === undefined) + if (resolveResult === undefined) continue; - if (resolved === null) { + if (resolveResult === null) { lastException = null; continue; } - return resolved; + return resolveResult; } if (lastException === undefined || lastException === null) return lastException; @@ -574,12 +595,12 @@ function resolvePackageTarget(packageJSONUrl, target, subpath, packageSubpath, const key = keys[i]; if (key === 'default' || conditions.has(key)) { const conditionalTarget = target[key]; - const resolved = resolvePackageTarget( + const resolveResult = resolvePackageTarget( packageJSONUrl, conditionalTarget, subpath, packageSubpath, base, pattern, internal, conditions); - if (resolved === undefined) + if (resolveResult === undefined) continue; - return resolved; + return resolveResult; } } return undefined; @@ -638,12 +659,15 @@ function packageExportsResolve( !StringPrototypeIncludes(packageSubpath, '*') && !StringPrototypeEndsWith(packageSubpath, '/')) { const target = exports[packageSubpath]; - const resolved = resolvePackageTarget( + const resolveResult = resolvePackageTarget( packageJSONUrl, target, '', packageSubpath, base, false, false, conditions ); - if (resolved === null || resolved === undefined) + + if (resolveResult == null) { throwExportsNotFound(packageSubpath, packageJSONUrl, base); - return { resolved, exact: true }; + } + + return { ...resolveResult, exact: true }; } let bestMatch = ''; @@ -679,14 +703,25 @@ function packageExportsResolve( if (bestMatch) { const target = exports[bestMatch]; const pattern = StringPrototypeIncludes(bestMatch, '*'); - const resolved = resolvePackageTarget(packageJSONUrl, target, - bestMatchSubpath, bestMatch, base, - pattern, false, conditions); - if (resolved === null || resolved === undefined) + const resolveResult = resolvePackageTarget( + packageJSONUrl, + target, + bestMatchSubpath, + bestMatch, + base, + pattern, + false, + conditions); + + if (resolveResult == null) { throwExportsNotFound(packageSubpath, packageJSONUrl, base); - if (!pattern) + } + + if (!pattern) { emitFolderMapDeprecation(bestMatch, packageJSONUrl, true, base); - return { resolved, exact: pattern }; + } + + return { ...resolveResult, exact: pattern }; } throwExportsNotFound(packageSubpath, packageJSONUrl, base); @@ -726,11 +761,12 @@ function packageImportsResolve(name, base, conditions) { if (ObjectPrototypeHasOwnProperty(imports, name) && !StringPrototypeIncludes(name, '*') && !StringPrototypeEndsWith(name, '/')) { - const resolved = resolvePackageTarget( + const resolveResult = resolvePackageTarget( packageJSONUrl, imports[name], '', name, base, false, true, conditions ); - if (resolved !== null) - return { resolved, exact: true }; + if (resolveResult != null) { + return { resolved: resolveResult.resolved, exact: true }; + } } else { let bestMatch = ''; let bestMatchSubpath; @@ -762,14 +798,15 @@ function packageImportsResolve(name, base, conditions) { if (bestMatch) { const target = imports[bestMatch]; const pattern = StringPrototypeIncludes(bestMatch, '*'); - const resolved = resolvePackageTarget(packageJSONUrl, target, - bestMatchSubpath, bestMatch, - base, pattern, true, - conditions); - if (resolved !== null) { + const resolveResult = resolvePackageTarget( + packageJSONUrl, target, + bestMatchSubpath, bestMatch, + base, pattern, true, + conditions); + if (resolveResult !== null) { if (!pattern) emitFolderMapDeprecation(bestMatch, packageJSONUrl, false, base); - return { resolved, exact: pattern }; + return { resolved: resolveResult.resolved, exact: pattern }; } } } @@ -833,7 +870,7 @@ function parsePackageName(specifier, base) { * @param {string} specifier * @param {string | URL | undefined} base * @param {Set} conditions - * @returns {URL} + * @returns {resolved: URL, format? : string} */ function packageResolve(specifier, base, conditions) { const { packageName, packageSubpath, isScoped } = @@ -846,8 +883,7 @@ function packageResolve(specifier, base, conditions) { if (packageConfig.name === packageName && packageConfig.exports !== undefined && packageConfig.exports !== null) { return packageExportsResolve( - packageJSONUrl, packageSubpath, packageConfig, base, conditions - ).resolved; + packageJSONUrl, packageSubpath, packageConfig, base, conditions); } } @@ -869,13 +905,26 @@ function packageResolve(specifier, base, conditions) { // Package match. const packageConfig = getPackageConfig(packageJSONPath, specifier, base); - if (packageConfig.exports !== undefined && packageConfig.exports !== null) + if (packageConfig.exports !== undefined && packageConfig.exports !== null) { return packageExportsResolve( - packageJSONUrl, packageSubpath, packageConfig, base, conditions - ).resolved; - if (packageSubpath === '.') - return legacyMainResolve(packageJSONUrl, packageConfig, base); - return new URL(packageSubpath, packageJSONUrl); + packageJSONUrl, packageSubpath, packageConfig, base, conditions); + } + + if (packageSubpath === '.') { + return { + resolved: legacyMainResolve( + packageJSONUrl, + packageConfig, + base), + ...(packageConfig.type !== 'none') && { format: packageConfig.type } + }; + } + + return { + resolved: new URL(packageSubpath, packageJSONUrl), + ...(packageConfig.type !== 'none') && { format: packageConfig.type } + }; + // Cross-platform root check. } while (packageJSONPath.length !== lastPath.length); @@ -912,12 +961,13 @@ function shouldBeTreatedAsRelativeOrAbsolutePath(specifier) { * @param {string} specifier * @param {string | URL | undefined} base * @param {Set} conditions - * @returns {URL} + * @returns {url: URL, format?: string} */ function moduleResolve(specifier, base, conditions) { // 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] === '#') { @@ -926,10 +976,13 @@ function moduleResolve(specifier, base, conditions) { try { resolved = new URL(specifier); } catch { - resolved = packageResolve(specifier, base, conditions); + ({ resolved, format } = packageResolve(specifier, base, conditions)); } } - return finalizeResolution(resolved, base); + return { + url: finalizeResolution(resolved, base), + ...(format != null) && { format } + }; } /** @@ -1040,8 +1093,14 @@ function defaultResolve(specifier, context = {}, defaultResolveUnused) { conditions = getConditionsSet(conditions); let url; + let format; try { - url = moduleResolve(specifier, parentURL, conditions); + ({ url, format } = + moduleResolve( + specifier, + parentURL, + conditions + )); } catch (error) { // Try to give the user a hint of what would have been the // resolved CommonJS module @@ -1077,7 +1136,10 @@ function defaultResolve(specifier, context = {}, defaultResolveUnused) { url.hash = old.hash; } - return { url: `${url}` }; + return { + url: `${url}`, + ...(format != null) && { format } + }; } module.exports = { diff --git a/test/es-module/test-esm-loader-resolve-type.mjs b/test/es-module/test-esm-loader-resolve-type.mjs new file mode 100644 index 00000000000000..f4bab3723d1f46 --- /dev/null +++ b/test/es-module/test-esm-loader-resolve-type.mjs @@ -0,0 +1,41 @@ +// Flags: --loader ./test/fixtures/es-module-loaders/hook-resolve-type.mjs +import { allowGlobals } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import { strict as assert } from 'assert'; +import * as fs from 'fs'; + +allowGlobals(global.getModuleTypeStats); + +const basePath = + new URL('./node_modules/', import.meta.url); + +const rel = (file) => new URL(file, basePath); +const createDir = (path) => { + if (!fs.existsSync(path)) { + fs.mkdirSync(path); + } +}; + +const moduleName = 'module-counter-by-type'; + +const moduleDir = rel(`${moduleName}`); +createDir(basePath); +createDir(moduleDir); +fs.cpSync( + fixtures.path('es-modules', moduleName), + moduleDir, + { recursive: true } +); + +const { importedESM: importedESMBefore, + importedCJS: importedCJSBefore } = global.getModuleTypeStats(); + +import(`${moduleName}`).finally(() => { + fs.rmSync(basePath, { recursive: true, force: true }); +}); + +const { importedESM: importedESMAfter, + importedCJS: importedCJSAfter } = global.getModuleTypeStats(); + +assert.strictEqual(importedESMBefore + 1, importedESMAfter); +assert.strictEqual(importedCJSBefore, importedCJSAfter); diff --git a/test/es-module/test-esm-resolve-type.js b/test/es-module/test-esm-resolve-type.js new file mode 100644 index 00000000000000..3cc484188ad513 --- /dev/null +++ b/test/es-module/test-esm-resolve-type.js @@ -0,0 +1,184 @@ +'use strict'; +// Flags: --expose-internals + +/** + * This test ensures defaultResolve returns the found module format in the + * return object in the form: + * { url: , format: <'module'|'commonjs'|undefined> }; + */ + +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const fixtures = require('../common/fixtures'); +const path = require('path'); +const fs = require('fs'); +const url = require('url'); + +if (!common.isMainThread) { + common.skip( + 'test-esm-resolve-type.js: process.chdir is not available in Workers' + ); +} + +const assert = require('assert'); +const { + defaultResolve: resolve +} = require('internal/modules/esm/resolve'); + +const rel = (file) => path.join(tmpdir.path, file); +const previousCwd = process.cwd(); +const nmDir = rel('node_modules'); +try { + tmpdir.refresh(); + process.chdir(tmpdir.path); + /** + * 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 ], + ].forEach((testVariant) => { + const [ testScript, expectedType ] = testVariant; + const resolvedPath = path.resolve(fixtures.path(testScript)); + const resolveResult = resolve(url.pathToFileURL(resolvedPath)); + assert.strictEqual(resolveResult.format, expectedType); + }); + + /** + * create a test module and try to resolve it by module name. + * check the result is as expected + */ + + [ [ 'test-module-mainjs', 'js', 'module', 'module'], + [ 'test-module-mainmjs', 'mjs', 'module', 'module'], + [ 'test-module-cjs', 'js', 'commonjs', 'commonjs'], + [ 'test-module-ne', 'js', undefined, undefined], + ].forEach((testVariant) => { + const [ moduleName, + moduleExtenstion, + moduleType, + expectedResolvedType ] = testVariant; + process.chdir(previousCwd); + tmpdir.refresh(); + process.chdir(tmpdir.path); + const createDir = (path) => { + if (!fs.existsSync(path)) { + fs.mkdirSync(path); + } + }; + + const mDir = rel(`node_modules/${moduleName}`); + const subDir = rel(`node_modules/${moduleName}/subdir`); + const pkg = rel(`node_modules/${moduleName}/package.json`); + const script = rel(`node_modules/${moduleName}/subdir/mainfile.${moduleExtenstion}`); + + createDir(nmDir); + createDir(mDir); + createDir(subDir); + const pkgJsonContent = { + ...(moduleType !== undefined) && { type: moduleType }, + main: `subdir/mainfile.${moduleExtenstion}` + }; + fs.writeFileSync(pkg, JSON.stringify(pkgJsonContent)); + fs.writeFileSync(script, + 'export function esm-resolve-tester() {return 42}'); + + const resolveResult = resolve(`${moduleName}`); + assert.strictEqual(resolveResult.format, expectedResolvedType); + + fs.rmSync(nmDir, { recursive: true, force: true }); + }); + + // Helpers + const createDir = (path) => { + if (!fs.existsSync(path)) { + fs.mkdirSync(path); + } + }; + + // 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' + }; + + 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')); +} finally { + process.chdir(previousCwd); + fs.rmSync(nmDir, { recursive: true, force: true }); +} diff --git a/test/fixtures/es-module-loaders/hook-resolve-type.mjs b/test/fixtures/es-module-loaders/hook-resolve-type.mjs new file mode 100644 index 00000000000000..48692ba4eec544 --- /dev/null +++ b/test/fixtures/es-module-loaders/hook-resolve-type.mjs @@ -0,0 +1,21 @@ +let importedESM = 0; +let importedCJS = 0; +global.getModuleTypeStats = () => { return {importedESM, importedCJS} }; + +export function load(url, context, next) { + return next(url, context, next); +} + +export function resolve(specifier, context, next) { + const nextResult = next(specifier, context); + const { format } = nextResult; + + if (format === 'module' || specifier.endsWith('.mjs')) { + importedESM++; + } else if (format == null || format === 'commonjs') { + importedCJS++; + } + + return nextResult; +} + diff --git a/test/fixtures/es-modules/module-counter-by-type/index.js b/test/fixtures/es-modules/module-counter-by-type/index.js new file mode 100644 index 00000000000000..901fd8dae0a419 --- /dev/null +++ b/test/fixtures/es-modules/module-counter-by-type/index.js @@ -0,0 +1,3 @@ +let dummy = 42; + +export {dummy}; diff --git a/test/fixtures/es-modules/module-counter-by-type/package.json b/test/fixtures/es-modules/module-counter-by-type/package.json new file mode 100644 index 00000000000000..c60bc2b004572a --- /dev/null +++ b/test/fixtures/es-modules/module-counter-by-type/package.json @@ -0,0 +1,4 @@ +{ + "type": "module", + "main": "index.js" +} diff --git a/test/fixtures/es-modules/package-without-pjson/index.js b/test/fixtures/es-modules/package-without-pjson/index.js new file mode 100644 index 00000000000000..29560bd838d029 --- /dev/null +++ b/test/fixtures/es-modules/package-without-pjson/index.js @@ -0,0 +1,7 @@ +const identifier = 'package-without-pjson'; + +const common = require('../common'); +common.requireNoPackageJSONAbove(); + +console.log(identifier); +module.exports = identifier;