From d9084d29fe18f36567baf5592e9a80cb401d33cd Mon Sep 17 00:00:00 2001 From: Jan Krems Date: Tue, 23 Jul 2019 21:25:24 -0700 Subject: [PATCH] module: unify package exports test for CJS and ESM Refs: https://github.com/nodejs/modules/issues/358 PR-URL: https://github.com/nodejs/node/pull/28831 Reviewed-By: Guy Bedford Reviewed-By: Rich Trott --- src/module_wrap.cc | 7 +- test/es-module/test-esm-exports.mjs | 111 ++++++++++++++---- .../node_modules/pkgexports/package.json | 1 - test/fixtures/pkgexports-missing.mjs | 11 -- test/fixtures/pkgexports.mjs | 15 ++- test/parallel/test-module-package-exports.js | 47 -------- 6 files changed, 102 insertions(+), 90 deletions(-) delete mode 100644 test/fixtures/pkgexports-missing.mjs delete mode 100644 test/parallel/test-module-package-exports.js diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 503ca8a858e2b9..522e7c2a4587f1 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -847,8 +847,11 @@ Maybe PackageExportsResolve(Environment* env, std::string target(*target_utf8, target_utf8.length()); if (target.back() == '/' && target.substr(0, 2) == "./") { std::string subpath = pkg_subpath.substr(best_match_str.length()); - URL target_url(target + subpath, pjson_url); - return FinalizeResolution(env, target_url, base); + URL url_prefix(target, pjson_url); + URL target_url(subpath, url_prefix); + if (target_url.path().find(url_prefix.path()) == 0) { + return FinalizeResolution(env, target_url, base); + } } } } diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index 38cd81511fe8ef..ca10097aa9ba27 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -1,29 +1,88 @@ // Flags: --experimental-modules import { mustCall } from '../common/index.mjs'; -import { ok, strictEqual } from 'assert'; - -import { asdf, asdf2, space } from '../fixtures/pkgexports.mjs'; -import { - loadMissing, - loadFromNumber, - loadDot, -} from '../fixtures/pkgexports-missing.mjs'; - -strictEqual(asdf, 'asdf'); -strictEqual(asdf2, 'asdf'); -strictEqual(space, 'encoded path'); - -loadMissing().catch(mustCall((err) => { - ok(err.message.toString().startsWith('Package exports')); - ok(err.message.toString().indexOf('do not define a \'./missing\' subpath')); -})); - -loadFromNumber().catch(mustCall((err) => { - ok(err.message.toString().startsWith('Package exports')); - ok(err.message.toString().indexOf('do not define a \'./missing\' subpath')); -})); - -loadDot().catch(mustCall((err) => { - ok(err.message.toString().startsWith('Cannot find main entry point')); -})); +import { ok, deepStrictEqual, strictEqual } from 'assert'; + +import { requireFixture, importFixture } from '../fixtures/pkgexports.mjs'; + +[requireFixture, importFixture].forEach((loadFixture) => { + const isRequire = loadFixture === requireFixture; + + const validSpecifiers = new Map([ + // A simple mapping of a path. + ['pkgexports/valid-cjs', { default: 'asdf' }], + // A directory mapping, pointing to the package root. + ['pkgexports/sub/asdf.js', { default: 'asdf' }], + // A mapping pointing to a file that needs special encoding (%20) in URLs. + ['pkgexports/space', { default: 'encoded path' }], + // Verifying that normal packages still work with exports turned on. + isRequire ? ['baz/index', { default: 'eye catcher' }] : [null], + ]); + for (const [validSpecifier, expected] of validSpecifiers) { + if (validSpecifier === null) continue; + + loadFixture(validSpecifier) + .then(mustCall((actual) => { + deepStrictEqual({ ...actual }, expected); + })); + } + + // There's no such export - so there's nothing to do. + loadFixture('pkgexports/missing').catch(mustCall((err) => { + strictEqual(err.code, 'ERR_PATH_NOT_EXPORTED'); + assertStartsWith(err.message, 'Package exports'); + assertIncludes(err.message, 'do not define a \'./missing\' subpath'); + })); + + // The file exists but isn't exported. The exports is a number which counts + // as a non-null value without any properties, just like `{}`. + loadFixture('pkgexports-number/hidden.js').catch(mustCall((err) => { + strictEqual(err.code, 'ERR_PATH_NOT_EXPORTED'); + assertStartsWith(err.message, 'Package exports'); + assertIncludes(err.message, 'do not define a \'./hidden.js\' subpath'); + })); + + // There's no main field so we won't find anything when importing the name. + // The fact that "." is mapped is ignored, it's not a valid main config. + loadFixture('pkgexports').catch(mustCall((err) => { + if (isRequire) { + strictEqual(err.code, 'MODULE_NOT_FOUND'); + assertStartsWith(err.message, 'Cannot find module \'pkgexports\''); + } else { + strictEqual(err.code, 'ERR_MODULE_NOT_FOUND'); + assertStartsWith(err.message, 'Cannot find main entry point'); + } + })); + + // Even though 'pkgexports/sub/asdf.js' works, alternate "path-like" variants + // do not to prevent confusion and accidental loopholes. + loadFixture('pkgexports/sub/./../asdf.js').catch(mustCall((err) => { + strictEqual(err.code, 'ERR_PATH_NOT_EXPORTED'); + assertStartsWith(err.message, 'Package exports'); + assertIncludes(err.message, + 'do not define a \'./sub/./../asdf.js\' subpath'); + })); + + // Covering out bases - not a file is still not a file after dir mapping. + loadFixture('pkgexports/sub/not-a-file.js').catch(mustCall((err) => { + if (isRequire) { + strictEqual(err.code, 'MODULE_NOT_FOUND'); + assertStartsWith(err.message, + 'Cannot find module \'pkgexports/sub/not-a-file.js\''); + } else { + strictEqual(err.code, 'ERR_MODULE_NOT_FOUND'); + // ESM currently returns a full file path + assertStartsWith(err.message, 'Cannot find module'); + } + })); +}); + +function assertStartsWith(actual, expected) { + const start = actual.toString().substr(0, expected.length); + strictEqual(start, expected); +} + +function assertIncludes(actual, expected) { + ok(actual.toString().indexOf(expected), + `${JSON.stringify(actual)} includes ${JSON.stringify(expected)}`); +} diff --git a/test/fixtures/node_modules/pkgexports/package.json b/test/fixtures/node_modules/pkgexports/package.json index b0c8867bb46fd1..97f07da85e2a61 100644 --- a/test/fixtures/node_modules/pkgexports/package.json +++ b/test/fixtures/node_modules/pkgexports/package.json @@ -2,7 +2,6 @@ "exports": { ".": "./asdf.js", "./space": "./sp%20ce.js", - "./asdf": "./asdf.js", "./valid-cjs": "./asdf.js", "./sub/": "./" } diff --git a/test/fixtures/pkgexports-missing.mjs b/test/fixtures/pkgexports-missing.mjs deleted file mode 100644 index 7d1d5b2e821629..00000000000000 --- a/test/fixtures/pkgexports-missing.mjs +++ /dev/null @@ -1,11 +0,0 @@ -export function loadMissing() { - return import('pkgexports/missing'); -} - -export function loadFromNumber() { - return import('pkgexports-number/hidden.js'); -} - -export function loadDot() { - return import('pkgexports'); -} diff --git a/test/fixtures/pkgexports.mjs b/test/fixtures/pkgexports.mjs index 8907ebcb0e253a..7d642c443e6b71 100644 --- a/test/fixtures/pkgexports.mjs +++ b/test/fixtures/pkgexports.mjs @@ -1,3 +1,12 @@ -export { default as asdf } from 'pkgexports/asdf'; -export { default as asdf2 } from 'pkgexports/sub/asdf.js'; -export { default as space } from 'pkgexports/space'; +import { fileURLToPath } from 'url'; +import { createRequire } from 'module'; + +const rawRequire = createRequire(fileURLToPath(import.meta.url)); + +export async function requireFixture(specifier) { + return { default: rawRequire(specifier ) }; +} + +export function importFixture(specifier) { + return import(specifier); +} diff --git a/test/parallel/test-module-package-exports.js b/test/parallel/test-module-package-exports.js deleted file mode 100644 index a1b9879448c17b..00000000000000 --- a/test/parallel/test-module-package-exports.js +++ /dev/null @@ -1,47 +0,0 @@ -// Flags: --experimental-exports -'use strict'; - -require('../common'); - -const assert = require('assert'); -const { createRequire } = require('module'); -const path = require('path'); - -const fixtureRequire = - createRequire(path.resolve(__dirname, '../fixtures/imaginary.js')); - -assert.strictEqual(fixtureRequire('pkgexports/valid-cjs'), 'asdf'); - -assert.strictEqual(fixtureRequire('baz/index'), 'eye catcher'); - -assert.strictEqual(fixtureRequire('pkgexports/sub/asdf.js'), 'asdf'); - -assert.strictEqual(fixtureRequire('pkgexports/space'), 'encoded path'); - -assert.throws( - () => fixtureRequire('pkgexports/not-a-known-entry'), - (e) => { - assert.strictEqual(e.code, 'ERR_PATH_NOT_EXPORTED'); - return true; - }); - -assert.throws( - () => fixtureRequire('pkgexports-number/hidden.js'), - (e) => { - assert.strictEqual(e.code, 'ERR_PATH_NOT_EXPORTED'); - return true; - }); - -assert.throws( - () => fixtureRequire('pkgexports/sub/not-a-file.js'), - (e) => { - assert.strictEqual(e.code, 'MODULE_NOT_FOUND'); - return true; - }); - -assert.throws( - () => fixtureRequire('pkgexports/sub/./../asdf.js'), - (e) => { - assert.strictEqual(e.code, 'ERR_PATH_NOT_EXPORTED'); - return true; - });