From f62fb7603af927b5fd014dba8ca84c3f5b2fd34d Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Tue, 17 Dec 2019 14:00:21 -0500 Subject: [PATCH] module: logical conditional exports ordering PR-URL: https://github.com/nodejs/node/pull/31008 Reviewed-By: Bradley Farias Reviewed-By: Jan Krems --- doc/api/esm.md | 48 +++++----- lib/internal/modules/cjs/loader.js | 68 ++++++++------ src/env.h | 2 - src/module_wrap.cc | 94 ++++++++++++------- test/es-module/test-esm-exports.mjs | 5 + .../pkgexports-numeric/package.json | 6 ++ .../node_modules/pkgexports/package.json | 15 ++- 7 files changed, 152 insertions(+), 86 deletions(-) create mode 100644 test/fixtures/node_modules/pkgexports-numeric/package.json diff --git a/doc/api/esm.md b/doc/api/esm.md index 381bdc8a053443..d48eb2ea9f7e19 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -343,25 +343,26 @@ Node.js and the browser can be written: When resolving the `"."` export, if no matching target is found, the `"main"` will be used as the final fallback. -The conditions supported in Node.js are matched in the following order: +The conditions supported in Node.js condition matching: -1. `"node"` - matched for any Node.js environment. Can be a CommonJS or ES +* `"default"` - the generic fallback that will always match. Can be a CommonJS + or ES module file. +* `"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._ +* `"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._ -2. `"require"` - matched when the package is loaded via `require()`. +* `"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. +Condition matching is applied in object order from first to last within the +`"exports"` object. + +> Setting the above conditions for a published package is not recommended until +> conditional exports have been unflagged to avoid breaking changes to packages. 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 @@ -369,7 +370,9 @@ hazard in that it can result in having two separate instances of the same package in use in an application, which can cause a number of bugs. Other conditions such as `"browser"`, `"electron"`, `"deno"`, `"react-native"`, -etc. could be defined in other runtimes or tools. +etc. could be defined in other runtimes or tools. Condition names must not start +with `"."` or be numbers. Further restrictions, definitions or guidance on +condition names may be provided in future. #### Exports Sugar @@ -1547,13 +1550,15 @@ _defaultEnv_ is the conditional environment name priority array, > 1. If _resolved_ is contained in _resolvedTarget_, then > 1. Return _resolved_. > 1. Otherwise, if _target_ is a non-null Object, then -> 1. If _target_ has an object key matching one of the names in _env_, then -> 1. Let _targetValue_ be the corresponding value of the first object key -> of _target_ in _env_. -> 1. Let _resolved_ be the result of **PACKAGE_EXPORTS_TARGET_RESOLVE** -> (_packageURL_, _targetValue_, _subpath_, _env_). -> 1. Assert: _resolved_ is a String. -> 1. Return _resolved_. +> 1. If _exports_ contains any index property keys, as defined in ECMA-262 +> [6.1.7 Array Index][], throw an _Invalid Package Configuration_ error. +> 1. For each property _p_ of _target_, in object insertion order as, +> 1. If _env_ contains an entry for _p_, then +> 1. Let _targetValue_ be the value of the _p_ property in _target_. +> 1. Let _resolved_ be the result of **PACKAGE_EXPORTS_TARGET_RESOLVE** +> (_packageURL_, _targetValue_, _subpath_, _env_). +> 1. Assert: _resolved_ is a String. +> 1. Return _resolved_. > 1. Otherwise, if _target_ is an Array, then > 1. For each item _targetValue_ in _target_, do > 1. If _targetValue_ is an Array, continue the loop. @@ -1649,3 +1654,4 @@ success! [special scheme]: https://url.spec.whatwg.org/#special-scheme [the official standard format]: https://tc39.github.io/ecma262/#sec-modules [transpiler loader example]: #esm_transpiler_loader +[6.1.7 Array Index]: https://tc39.es/ecma262/#integer-index diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 7ebbaad20cdbeb..f25339bf666f9e 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -26,13 +26,16 @@ const { Error, JSONParse, Map, + Number, ObjectCreate, ObjectDefineProperty, ObjectFreeze, + ObjectIs, ObjectKeys, ObjectPrototypeHasOwnProperty, ReflectSet, SafeMap, + String, StringPrototypeIndexOf, StringPrototypeMatch, StringPrototypeSlice, @@ -554,6 +557,18 @@ function resolveExports(nmPath, request, absoluteRequest) { return path.resolve(nmPath, request); } +function isArrayIndex(p) { + assert(typeof p === 'string'); + const n = Number(p); + if (String(n) !== p) + return false; + if (ObjectIs(n, +0)) + return true; + if (!Number.isInteger(n)) + return false; + return n >= 0 && n < (2 ** 32) - 1; +} + function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { if (typeof target === 'string') { if (target.startsWith('./') && @@ -584,34 +599,33 @@ function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { } } } else if (typeof target === 'object' && target !== null) { - if (experimentalConditionalExports && - ObjectPrototypeHasOwnProperty(target, 'node')) { - try { - const result = resolveExportsTarget(pkgPath, target.node, subpath, - basePath, mappingKey); - emitExperimentalWarning('Conditional exports'); - return result; - } catch (e) { - if (e.code !== 'MODULE_NOT_FOUND') throw e; - } - } - if (experimentalConditionalExports && - ObjectPrototypeHasOwnProperty(target, 'require')) { - try { - const result = resolveExportsTarget(pkgPath, target.require, subpath, - basePath, mappingKey); - emitExperimentalWarning('Conditional exports'); - return result; - } catch (e) { - if (e.code !== 'MODULE_NOT_FOUND') throw e; - } + const keys = ObjectKeys(target); + if (keys.some(isArrayIndex)) { + throw new ERR_INVALID_PACKAGE_CONFIG(basePath, '"exports" cannot ' + + 'contain numeric property keys.'); } - if (ObjectPrototypeHasOwnProperty(target, 'default')) { - try { - return resolveExportsTarget(pkgPath, target.default, subpath, - basePath, mappingKey); - } catch (e) { - if (e.code !== 'MODULE_NOT_FOUND') throw e; + for (const p of keys) { + switch (p) { + case 'node': + case 'require': + if (!experimentalConditionalExports) + continue; + try { + emitExperimentalWarning('Conditional exports'); + const result = resolveExportsTarget(pkgPath, target[p], subpath, + basePath, mappingKey); + return result; + } catch (e) { + if (e.code !== 'MODULE_NOT_FOUND') throw e; + } + break; + case 'default': + try { + return resolveExportsTarget(pkgPath, target.default, subpath, + basePath, mappingKey); + } catch (e) { + if (e.code !== 'MODULE_NOT_FOUND') throw e; + } } } } diff --git a/src/env.h b/src/env.h index 8823b825c2b3b1..1569208caf4ae9 100644 --- a/src/env.h +++ b/src/env.h @@ -202,7 +202,6 @@ constexpr size_t kFsStatsBufferLength = V(crypto_rsa_pss_string, "rsa-pss") \ V(cwd_string, "cwd") \ V(data_string, "data") \ - V(default_string, "default") \ V(dest_string, "dest") \ V(destroyed_string, "destroyed") \ V(detached_string, "detached") \ @@ -257,7 +256,6 @@ constexpr size_t kFsStatsBufferLength = V(http_1_1_string, "http/1.1") \ V(identity_string, "identity") \ 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 74eacf198f7b84..ff6f0db651a7df 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -12,7 +12,6 @@ #include // S_IFDIR #include -#include // PATH_MAX namespace node { namespace loader { @@ -908,6 +907,25 @@ Maybe ResolveExportsTargetString(Environment* env, return Just(subpath_resolved); } +bool IsArrayIndex(Environment* env, Local p) { + Local context = env->context(); + Local p_str = p->ToString(context).ToLocalChecked(); + double n_dbl = static_cast(p_str->NumberValue(context).FromJust()); + Local n = Number::New(env->isolate(), n_dbl); + Local cmp_str = n->ToString(context).ToLocalChecked(); + if (!p_str->Equals(context, cmp_str).FromJust()) { + return false; + } + if (n_dbl == 0 && std::signbit(n_dbl) == false) { + return true; + } + Local cmp_integer; + if (!n->ToInteger(context).ToLocal(&cmp_integer)) { + return false; + } + return n_dbl > 0 && n_dbl < (2 ^ 32) - 1; +} + Maybe ResolveExportsTarget(Environment* env, const URL& pjson_url, Local target, @@ -953,44 +971,50 @@ Maybe ResolveExportsTarget(Environment* env, return Nothing(); } else if (target->IsObject()) { Local target_obj = target.As(); - bool matched = false; + Local target_obj_keys = + target_obj->GetOwnPropertyNames(context).ToLocalChecked(); Local conditionalTarget; - if (env->options()->experimental_conditional_exports && - target_obj->HasOwnProperty(context, env->node_string()).FromJust()) { - matched = true; - conditionalTarget = - target_obj->Get(context, env->node_string()).ToLocalChecked(); - Maybe resolved = ResolveExportsTarget(env, pjson_url, - conditionalTarget, subpath, pkg_subpath, base, false); - if (!resolved.IsNothing()) { - ProcessEmitExperimentalWarning(env, "Conditional exports"); - return resolved; + bool matched = false; + for (uint32_t i = 0; i < target_obj_keys->Length(); ++i) { + Local key = + target_obj_keys->Get(context, i).ToLocalChecked(); + if (IsArrayIndex(env, key)) { + const std::string msg = "Invalid package config for " + + pjson_url.ToFilePath() + ", \"exports\" cannot contain numeric " + + "property keys."; + node::THROW_ERR_INVALID_PACKAGE_CONFIG(env, msg.c_str()); + return Nothing(); } } - 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, + for (uint32_t i = 0; i < target_obj_keys->Length(); ++i) { + Local key = target_obj_keys->Get(context, i).ToLocalChecked(); + Utf8Value key_utf8(env->isolate(), + key->ToString(context).ToLocalChecked()); + std::string key_str(*key_utf8, key_utf8.length()); + if (key_str == "node" || key_str == "import") { + if (!env->options()->experimental_conditional_exports) continue; + matched = true; + conditionalTarget = target_obj->Get(context, key).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 = - target_obj->Get(context, env->default_string()).ToLocalChecked(); - Maybe resolved = ResolveExportsTarget(env, pjson_url, + if (!resolved.IsNothing()) { + ProcessEmitExperimentalWarning(env, "Conditional exports"); + return resolved; + } + } else if (key_str == "default") { + matched = true; + conditionalTarget = target_obj->Get(context, key).ToLocalChecked(); + Maybe resolved = ResolveExportsTarget(env, pjson_url, conditionalTarget, subpath, pkg_subpath, base, false); - if (!resolved.IsNothing()) { - return resolved; + if (!resolved.IsNothing()) { + ProcessEmitExperimentalWarning(env, "Conditional exports"); + return resolved; + } } } if (matched && throw_invalid) { Maybe resolved = ResolveExportsTarget(env, pjson_url, - conditionalTarget, subpath, pkg_subpath, base, true); + conditionalTarget, subpath, pkg_subpath, base, true); CHECK(resolved.IsNothing()); return Nothing(); } @@ -1013,8 +1037,8 @@ Maybe IsConditionalExportsMainSugar(Environment* env, exports_obj->GetOwnPropertyNames(context).ToLocalChecked(); bool isConditionalSugar = false; for (uint32_t i = 0; i < keys->Length(); ++i) { - Local key = keys->Get(context, i).ToLocalChecked().As(); - Utf8Value key_utf8(env->isolate(), key); + Local key = keys->Get(context, i).ToLocalChecked(); + Utf8Value key_utf8(env->isolate(), key->ToString(context).ToLocalChecked()); bool curIsConditionalSugar = key_utf8.length() == 0 || key_utf8[0] != '.'; if (i == 0) { isConditionalSugar = curIsConditionalSugar; @@ -1122,13 +1146,13 @@ Maybe PackageExportsResolve(Environment* env, Local keys = exports_obj->GetOwnPropertyNames(context).ToLocalChecked(); for (uint32_t i = 0; i < keys->Length(); ++i) { - Local key = keys->Get(context, i).ToLocalChecked().As(); - Utf8Value key_utf8(isolate, key); + Local key = keys->Get(context, i).ToLocalChecked(); + Utf8Value key_utf8(isolate, key->ToString(context).ToLocalChecked()); std::string key_str(*key_utf8, key_utf8.length()); if (key_str.back() != '/') continue; if (pkg_subpath.substr(0, key_str.length()) == key_str && key_str.length() > best_match_str.length()) { - best_match = key; + best_match = key->ToString(context).ToLocalChecked(); best_match_str = key_str; } } diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index d55d8ffac47f8c..96f33978968612 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -124,6 +124,11 @@ import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js'; 'ERR_MODULE_NOT_FOUND'); })); + // Package export with numeric index properties must throw a validation error + loadFixture('pkgexports-numeric').catch(mustCall((err) => { + strictEqual(err.code, 'ERR_INVALID_PACKAGE_CONFIG'); + })); + // Sugar conditional exports main mixed failure case loadFixture('pkgexports-sugar-fail').catch(mustCall((err) => { strictEqual(err.code, 'ERR_INVALID_PACKAGE_CONFIG'); diff --git a/test/fixtures/node_modules/pkgexports-numeric/package.json b/test/fixtures/node_modules/pkgexports-numeric/package.json new file mode 100644 index 00000000000000..6d20abe1d59a63 --- /dev/null +++ b/test/fixtures/node_modules/pkgexports-numeric/package.json @@ -0,0 +1,6 @@ +{ + "main": "./main.cjs", + "exports": { + "0": "./should-throw" + } +} diff --git a/test/fixtures/node_modules/pkgexports/package.json b/test/fixtures/node_modules/pkgexports/package.json index f7adc813ac81cc..02e06f0ebe5f3c 100644 --- a/test/fixtures/node_modules/pkgexports/package.json +++ b/test/fixtures/node_modules/pkgexports/package.json @@ -18,7 +18,20 @@ "./nofallback1": [], "./nofallback2": [null, {}, "builtin:x"], "./nodemodules": "./node_modules/internalpkg/x.js", - "./condition": [{ "require": "./sp ce.js" }, "./asdf.js"], + "./condition": [{ + "import": "///overridden", + "require": { + "require": { + "nomatch": "./nothing.js" + }, + "default": "./sp ce.js" + }, + "default": "./asdf.js", + "node": "./lib/hole.js", + "import": { + "nomatch": "./nothing.js" + } + }], "./resolve-self": { "require": "./resolve-self.js", "import": "./resolve-self.mjs"