module: implement logical conditional exports ordering

doc/api/esm.md

@@ -343,33 +343,36 @@ 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
 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

lib/internal/modules/cjs/loader.js

@@ -26,16 +26,19 @@ const {
  Error,
  JSONParse,
  Map,
+ Number,
  ObjectCreate,
  ObjectDefineProperty,
  ObjectFreeze,
  ObjectGetOwnPropertyDescriptor,
  ObjectGetPrototypeOf,
+ ObjectIs,
  ObjectKeys,
  ObjectPrototypeHasOwnProperty,
  ObjectSetPrototypeOf,
  ReflectSet,
  SafeMap,
+ String,
  StringPrototypeIndexOf,
  StringPrototypeMatch,
  StringPrototypeSlice,
@@ -557,6 +560,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('./') &&
@@ -587,34 +602,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;
+ }
  }
  }
  }

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") \

src/module_wrap.cc

@@ -1,3 +1,4 @@
+#define V8_ENABLE_CHECKS 1
 #include "module_wrap.h"
 
 #include "env.h"
@@ -12,7 +13,6 @@
 #include <sys/stat.h> // S_IFDIR
 
 #include <algorithm>
-#include <climits> // PATH_MAX
 
 namespace node {
 namespace loader {
@@ -908,6 +908,25 @@ Maybe<URL> ResolveExportsTargetString(Environment* env,
  return Just(subpath_resolved);
 }
 
+bool IsArrayIndex(Environment* env, Local<Value> p) {
+ Local<Context> context = env->context();
+ Local<String> p_str = p->ToString(context).ToLocalChecked();
+ double n_dbl = static_cast<double>(p_str->NumberValue(context).FromJust());
+ Local<Number> n = Number::New(env->isolate(), n_dbl);
+ Local<String> 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<Integer> cmp_integer;
+ if (!n->ToInteger(context).ToLocal(&cmp_integer)) {
+ return false;
+ }
+ return n_dbl > 0 && n_dbl < (2 ^ 32) - 1;
+}
+
 Maybe<URL> ResolveExportsTarget(Environment* env,
  const URL& pjson_url,
  Local<Value> target,
@@ -953,44 +972,50 @@ Maybe<URL> ResolveExportsTarget(Environment* env,
  return Nothing<URL>();
  } else if (target->IsObject()) {
  Local<Object> target_obj = target.As<Object>();
- bool matched = false;
+ Local<Array> target_obj_keys =
+ target_obj->GetOwnPropertyNames(context).ToLocalChecked();
  Local<Value> 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<URL> 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<Value> 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<URL>();
  }
  }
- 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<URL> resolved = ResolveExportsTarget(env, pjson_url,
+ for (uint32_t i = 0; i < target_obj_keys->Length(); ++i) {
+ Local<Value> 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<URL> 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<URL> 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<URL> 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<URL> resolved = ResolveExportsTarget(env, pjson_url,
-  conditionalTarget, subpath, pkg_subpath, base, true);
+ conditionalTarget, subpath, pkg_subpath, base, true);
  CHECK(resolved.IsNothing());
  return Nothing<URL>();
  }
@@ -1013,8 +1038,8 @@ Maybe<bool> IsConditionalExportsMainSugar(Environment* env,
  exports_obj->GetOwnPropertyNames(context).ToLocalChecked();
  bool isConditionalSugar = false;
  for (uint32_t i = 0; i < keys->Length(); ++i) {
- Local<String> key = keys->Get(context, i).ToLocalChecked().As<String>();
- Utf8Value key_utf8(env->isolate(), key);
+ Local<Value> 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 +1147,13 @@ Maybe<URL> PackageExportsResolve(Environment* env,
  Local<Array> keys =
  exports_obj->GetOwnPropertyNames(context).ToLocalChecked();
  for (uint32_t i = 0; i < keys->Length(); ++i) {
- Local<String> key = keys->Get(context, i).ToLocalChecked().As<String>();
- Utf8Value key_utf8(isolate, key);
+ Local<Value> 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;
  }
  }

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');