From f54e305fec7311a3006eaccd463e5e13682003fc Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Fri, 26 Nov 2021 12:09:30 +0100 Subject: [PATCH 1/2] loader: return package format from defaultResolve if known 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 --- lib/internal/modules/cjs/loader.js | 4 +- lib/internal/modules/esm/resolve.js | 158 ++++++++++----- .../test-esm-loader-resolve-type.mjs | 42 ++++ 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 + 8 files changed, 373 insertions(+), 50 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/cjs/loader.js b/lib/internal/modules/cjs/loader.js index e0f40ffa2ecf50..63f224acb5c184 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), parentPath, pkgPath); + pathToFileURL(parentPath), cjsConditions).resolved, 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), null, pkgPath); + cjsConditions).resolved, null, pkgPath); } catch (e) { if (e.code === 'ERR_MODULE_NOT_FOUND') throw createEsmNotFoundErr(request, pkgPath + '/package.json'); diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 50729350053fe2..9e05b1c9c97379 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -463,6 +463,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); @@ -478,7 +495,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); @@ -494,7 +512,7 @@ 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)) { const request = pattern ? @@ -502,10 +520,13 @@ function resolvePackageTargetString( throwInvalidSubpath(request, 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)); } /** @@ -531,9 +552,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) { @@ -542,13 +563,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; @@ -567,12 +588,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; @@ -631,12 +652,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; + } + + return resolveResult; } let bestMatch = ''; @@ -672,12 +696,20 @@ function packageExportsResolve( if (bestMatch) { const target = exports[bestMatch]; - const resolved = resolvePackageTarget(packageJSONUrl, target, - bestMatchSubpath, bestMatch, base, - true, false, conditions); - if (resolved === null || resolved === undefined) + const resolveResult = resolvePackageTarget( + packageJSONUrl, + target, + bestMatchSubpath, + bestMatch, + base, + true, + false, + conditions); + + if (resolveResult == null) { throwExportsNotFound(packageSubpath, packageJSONUrl, base); - return resolved; + } + return resolveResult; } throwExportsNotFound(packageSubpath, packageJSONUrl, base); @@ -717,11 +749,12 @@ function packageImportsResolve(name, base, conditions) { if (imports) { if (ObjectPrototypeHasOwnProperty(imports, name) && !StringPrototypeIncludes(name, '*')) { - const resolved = resolvePackageTarget( + const resolveResult = resolvePackageTarget( packageJSONUrl, imports[name], '', name, base, false, true, conditions ); - if (resolved !== null && resolved !== undefined) - return resolved; + if (resolveResult != null) { + return resolveResult.resolved; + } } else { let bestMatch = ''; let bestMatchSubpath; @@ -747,11 +780,13 @@ function packageImportsResolve(name, base, conditions) { if (bestMatch) { const target = imports[bestMatch]; - const resolved = resolvePackageTarget(packageJSONUrl, target, - bestMatchSubpath, bestMatch, - base, true, true, conditions); - if (resolved !== null && resolved !== undefined) - return resolved; + const resolveResult = resolvePackageTarget(packageJSONUrl, target, + bestMatchSubpath, + bestMatch, base, true, + true, conditions); + if (resolveResult != null) { + return resolveResult.resolved; + } } } } @@ -810,11 +845,11 @@ 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) { if (NativeModule.canBeRequiredByUsers(specifier)) - return new URL('node:' + specifier); + return { resolved: new URL('node:' + specifier) }; const { packageName, packageSubpath, isScoped } = parsePackageName(specifier, base); @@ -826,8 +861,7 @@ function packageResolve(specifier, base, conditions) { if (packageConfig.name === packageName && packageConfig.exports !== undefined && packageConfig.exports !== null) { return packageExportsResolve( - packageJSONUrl, packageSubpath, packageConfig, base, conditions - ); + packageJSONUrl, packageSubpath, packageConfig, base, conditions); } } @@ -849,13 +883,24 @@ 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 - ); - 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); @@ -893,12 +938,13 @@ function shouldBeTreatedAsRelativeOrAbsolutePath(specifier) { * @param {string | URL | undefined} base * @param {Set} conditions * @param {boolean} preserveSymlinks - * @returns {URL} + * @returns {url: URL, format?: string} */ 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] === '#') { @@ -907,12 +953,19 @@ function moduleResolve(specifier, base, conditions, preserveSymlinks) { try { resolved = new URL(specifier); } catch { - resolved = packageResolve(specifier, base, conditions); + ({ resolved, format } = packageResolve(specifier, base, conditions)); } } - if (resolved.protocol !== 'file:') - return resolved; - return finalizeResolution(resolved, base, preserveSymlinks); + if (resolved.protocol !== 'file:') { + return { + url: resolved + }; + } + + return { + url: finalizeResolution(resolved, base, preserveSymlinks), + ...(format != null) && { format } + }; } /** @@ -1001,9 +1054,15 @@ function defaultResolve(specifier, context = {}, defaultResolveUnused) { conditions = getConditionsSet(conditions); let url; + let format; try { - url = moduleResolve(specifier, parentURL, conditions, - isMain ? preserveSymlinksMain : preserveSymlinks); + ({ url, format } = + 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 @@ -1031,7 +1090,10 @@ function defaultResolve(specifier, context = {}, defaultResolveUnused) { url.protocol !== 'node:') throw new ERR_UNSUPPORTED_ESM_URL_SCHEME(url); - 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..9519c2b9da8bcc --- /dev/null +++ b/test/es-module/test-esm-loader-resolve-type.mjs @@ -0,0 +1,42 @@ +// 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'; +import { fileURLToPath } from 'url'; + +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(fileURLToPath(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; From 2d2c4754322117dd5ccf362206e45be42c0edfb8 Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Mon, 13 Dec 2021 16:28:53 +0100 Subject: [PATCH 2/2] fixup! loader: return package format from defaultResolve if known --- test/es-module/test-esm-loader-resolve-type.mjs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/es-module/test-esm-loader-resolve-type.mjs b/test/es-module/test-esm-loader-resolve-type.mjs index 9519c2b9da8bcc..f4bab3723d1f46 100644 --- a/test/es-module/test-esm-loader-resolve-type.mjs +++ b/test/es-module/test-esm-loader-resolve-type.mjs @@ -3,7 +3,6 @@ import { allowGlobals } from '../common/index.mjs'; import * as fixtures from '../common/fixtures.mjs'; import { strict as assert } from 'assert'; import * as fs from 'fs'; -import { fileURLToPath } from 'url'; allowGlobals(global.getModuleTypeStats); @@ -32,7 +31,7 @@ const { importedESM: importedESMBefore, importedCJS: importedCJSBefore } = global.getModuleTypeStats(); import(`${moduleName}`).finally(() => { - fs.rmSync(fileURLToPath(basePath), { recursive: true, force: true }); + fs.rmSync(basePath, { recursive: true, force: true }); }); const { importedESM: importedESMAfter,