From 357a99293e0132ed96afde56df5ce5aea2b550cd Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 4 Dec 2019 21:14:48 -0500 Subject: [PATCH] module: conditional exports import condition PR-URL: https://github.com/nodejs/node/pull/30799 Reviewed-By: Jan Krems Reviewed-By: Myles Borins --- doc/api/esm.md | 49 +++++++++++-------- doc/api/modules.md | 3 +- lib/internal/modules/cjs/loader.js | 8 +-- src/env.h | 1 + src/module_wrap.cc | 11 +++++ test/es-module/test-esm-exports.mjs | 13 ++++- .../node_modules/pkgexports-main/main.cjs | 1 + .../node_modules/pkgexports-main/module.mjs | 1 + .../node_modules/pkgexports-main/package.json | 6 +++ .../pkgexports-sugar-fail/package.json | 2 +- .../pkgexports-sugar2/package.json | 2 +- 11 files changed, 67 insertions(+), 30 deletions(-) create mode 100644 test/fixtures/node_modules/pkgexports-main/main.cjs create mode 100644 test/fixtures/node_modules/pkgexports-main/module.mjs create mode 100644 test/fixtures/node_modules/pkgexports-main/package.json diff --git a/doc/api/esm.md b/doc/api/esm.md index f834f29fbbee85..5245cef465e3c7 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -329,8 +329,8 @@ Node.js and the browser can be written: "main": "./index.js", "exports": { "./feature": { - "browser": "./feature-browser.js", - "default": "./feature-default.js" + "import": "./feature-default.js", + "browser": "./feature-browser.js" } } } @@ -341,16 +341,24 @@ will be used as the final fallback. The conditions supported in Node.js are matched in the following order: -1. `"require"` - matched when the package is loaded via `require()`. - _This is currently only supported behind the - `--experimental-conditional-exports` flag._ -2. `"node"` - matched for any Node.js environment. Can be a CommonJS or ES +1. `"node"` - matched for any Node.js environment. Can be a CommonJS or ES module file. _This is currently only supported behind the - `--experimental-conditional-exports` flag._ -3. `"default"` - the generic fallback that will always match if no other + `--experimental-conditional-exports` flag._ +2. `"require"` - matched when the package is loaded via `require()`. + _This is currently only supported behind the + `--experimental-conditional-exports` flag._ +3. `"import"` - matched when the package is loaded via `import` or + `import()`. Can be any module format, this field does not set the type + interpretation. _This is currently only supported behind the + `--experimental-conditional-exports` flag._ +4. `"default"` - the generic fallback that will always match if no other more specific condition is matched first. Can be a CommonJS or ES module file. +> Setting any of the above flagged conditions for a published package is not +> recommended until they are unflagged to avoid breaking changes to packages in +> future. + Using the `"require"` condition it is possible to define a package that will have a different exported value for CommonJS and ES modules, which can be a hazard in that it can result in having two separate instances of the same @@ -394,8 +402,8 @@ from exports subpaths. { "exports": { ".": { - "require": "./main.cjs", - "default": "./main.js" + "import": "./main.js", + "require": "./main.cjs" } } } @@ -407,8 +415,8 @@ can be written: ```js { "exports": { - "require": "./main.cjs", - "default": "./main.js" + "import": "./main.js", + "require": "./main.cjs" } } ``` @@ -422,8 +430,8 @@ thrown: // Throws on resolution! "exports": { "./feature": "./lib/feature.js", - "require": "./main.cjs", - "default": "./main.js" + "import": "./main.js", + "require": "./main.cjs" } } ``` @@ -508,9 +516,8 @@ ES module wrapper is used for `import` and the CommonJS entry point for `require`. > Note: While `--experimental-conditional-exports` is flagged, a package -> using this pattern will throw when loaded via `require()` in modern -> Node.js, unless package consumers use the `--experimental-conditional-exports` -> flag. +> using this pattern will throw when loaded unless package consumers use the +> `--experimental-conditional-exports` flag. ```js @@ -520,7 +527,7 @@ ES module wrapper is used for `import` and the CommonJS entry point for "main": "./index.cjs", "exports": { "require": "./index.cjs", - "default": "./wrapper.mjs" + "import": "./wrapper.mjs" } } ``` @@ -605,8 +612,8 @@ CommonJS and ES module entry points directly (requires "type": "module", "main": "./index.cjs", "exports": { - "require": "./index.cjs", - "default": "./index.mjs" + "import": "./index.mjs", + "require": "./index.cjs" } } ``` @@ -1149,7 +1156,7 @@ of these top-level routines unless stated otherwise. _isMain_ is **true** when resolving the Node.js application entry point. _defaultEnv_ is the conditional environment name priority array, -`["node", "default"]`. +`["node", "import"]`.
Resolver algorithm specification diff --git a/doc/api/modules.md b/doc/api/modules.md index 42f42c07fd7b6d..5653fe8f4245bd 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -241,7 +241,8 @@ RESOLVE_BARE_SPECIFIER(DIR, X) g. If no such key can be found, throw "not found". h. let RESOLVED_URL = PACKAGE_EXPORTS_TARGET_RESOLVE(pathToFileURL(DIR/name), exports[key], - subpath.slice(key.length)), as defined in the ESM resolver. + subpath.slice(key.length), ["node", "require"]), as defined in the ESM + resolver. i. return fileURLToPath(RESOLVED_URL) 3. return DIR/X ``` diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index f4112a6a0e6717..acdd14b959895b 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -585,9 +585,9 @@ function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { } } else if (typeof target === 'object' && target !== null) { if (experimentalConditionalExports && - ObjectPrototypeHasOwnProperty(target, 'require')) { + ObjectPrototypeHasOwnProperty(target, 'node')) { try { - const result = resolveExportsTarget(pkgPath, target.require, subpath, + const result = resolveExportsTarget(pkgPath, target.node, subpath, basePath, mappingKey); emitExperimentalWarning('Conditional exports'); return result; @@ -596,9 +596,9 @@ function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { } } if (experimentalConditionalExports && - ObjectPrototypeHasOwnProperty(target, 'node')) { + ObjectPrototypeHasOwnProperty(target, 'require')) { try { - const result = resolveExportsTarget(pkgPath, target.node, subpath, + const result = resolveExportsTarget(pkgPath, target.require, subpath, basePath, mappingKey); emitExperimentalWarning('Conditional exports'); return result; diff --git a/src/env.h b/src/env.h index b3f1243f77584b..8cd56b4dd66343 100644 --- a/src/env.h +++ b/src/env.h @@ -255,6 +255,7 @@ constexpr size_t kFsStatsBufferLength = V(hostmaster_string, "hostmaster") \ V(http_1_1_string, "http/1.1") \ V(ignore_string, "ignore") \ + V(import_string, "import") \ V(infoaccess_string, "infoAccess") \ V(inherit_string, "inherit") \ V(input_string, "input") \ diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 2fa8ba498edcb7..568e1ad2fdbba0 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -967,6 +967,17 @@ Maybe ResolveExportsTarget(Environment* env, return resolved; } } + if (env->options()->experimental_conditional_exports && + target_obj->HasOwnProperty(context, env->import_string()).FromJust()) { + matched = true; + conditionalTarget = + target_obj->Get(context, env->import_string()).ToLocalChecked(); + Maybe resolved = ResolveExportsTarget(env, pjson_url, + conditionalTarget, subpath, pkg_subpath, base, false); + if (!resolved.IsNothing()) { + return resolved; + } + } if (target_obj->HasOwnProperty(context, env->default_string()).FromJust()) { matched = true; conditionalTarget = diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index 73a86793a23863..fd85e155b0af21 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -31,7 +31,7 @@ import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js'; ['pkgexports-sugar', { default: 'main' }], // Conditional object exports sugar ['pkgexports-sugar2', isRequire ? { default: 'not-exported' } : - { default: 'main' }] + { default: 'main' }], ]); for (const [validSpecifier, expected] of validSpecifiers) { @@ -51,7 +51,7 @@ import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js'; ['pkgexports-number/hidden.js', './hidden.js'], // Sugar cases still encapsulate ['pkgexports-sugar/not-exported.js', './not-exported.js'], - ['pkgexports-sugar2/not-exported.js', './not-exported.js'] + ['pkgexports-sugar2/not-exported.js', './not-exported.js'], ]); const invalidExports = new Map([ @@ -97,6 +97,15 @@ import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js'; })); } + // Conditional export, even with no match, should still be used instead + // of falling back to main + if (isRequire) { + loadFixture('pkgexports-main').catch(mustCall((err) => { + strictEqual(err.code, 'MODULE_NOT_FOUND'); + assertStartsWith(err.message, 'No valid export'); + })); + } + // Covering out bases - not a file is still not a file after dir mapping. loadFixture('pkgexports/sub/not-a-file.js').catch(mustCall((err) => { strictEqual(err.code, (isRequire ? '' : 'ERR_') + 'MODULE_NOT_FOUND'); diff --git a/test/fixtures/node_modules/pkgexports-main/main.cjs b/test/fixtures/node_modules/pkgexports-main/main.cjs new file mode 100644 index 00000000000000..b2825bd3c9949b --- /dev/null +++ b/test/fixtures/node_modules/pkgexports-main/main.cjs @@ -0,0 +1 @@ +module.exports = 'cjs'; diff --git a/test/fixtures/node_modules/pkgexports-main/module.mjs b/test/fixtures/node_modules/pkgexports-main/module.mjs new file mode 100644 index 00000000000000..bd1596224653a3 --- /dev/null +++ b/test/fixtures/node_modules/pkgexports-main/module.mjs @@ -0,0 +1 @@ +export default 'module'; diff --git a/test/fixtures/node_modules/pkgexports-main/package.json b/test/fixtures/node_modules/pkgexports-main/package.json new file mode 100644 index 00000000000000..5546af0c84d7fd --- /dev/null +++ b/test/fixtures/node_modules/pkgexports-main/package.json @@ -0,0 +1,6 @@ +{ + "main": "./main.cjs", + "exports": { + "module": "./module.mjs" + } +} diff --git a/test/fixtures/node_modules/pkgexports-sugar-fail/package.json b/test/fixtures/node_modules/pkgexports-sugar-fail/package.json index 0fb05a427a76e2..bfa040647c509e 100644 --- a/test/fixtures/node_modules/pkgexports-sugar-fail/package.json +++ b/test/fixtures/node_modules/pkgexports-sugar-fail/package.json @@ -1,6 +1,6 @@ { "exports": { - "default": "./main.js", + "module": "./main.js", "./main": "./main.js" } } diff --git a/test/fixtures/node_modules/pkgexports-sugar2/package.json b/test/fixtures/node_modules/pkgexports-sugar2/package.json index 139b06665d85e0..bef4889bea8930 100644 --- a/test/fixtures/node_modules/pkgexports-sugar2/package.json +++ b/test/fixtures/node_modules/pkgexports-sugar2/package.json @@ -1,6 +1,6 @@ { "exports": { "require": "./not-exported.js", - "default": "./main.js" + "import": "./main.js" } }