module: logical conditional exports ordering

guybedford · MylesBorins · commit fae517a0d7a5 · 2020-01-28T00:32:53.000-05:00
PR-URL: #31008 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>
diff --git a/doc/api/esm.md b/doc/api/esm.md
@@ -349,33 +349,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
 
@@ -1557,13 +1560,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.
@@ -1659,3 +1664,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
@@ -26,13 +26,16 @@ const {
   Error,
   JSONParse,
   Map,
+  Number,
   ObjectCreate,
   ObjectDefineProperty,
   ObjectFreeze,
+  ObjectIs,
   ObjectKeys,
   ObjectPrototypeHasOwnProperty,
   ReflectSet,
   SafeMap,
+  String,
   StringPrototypeIndexOf,
   StringPrototypeMatch,
   StringPrototypeSlice,
@@ -555,6 +558,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('./') &&
@@ -585,34 +600,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
@@ -203,7 +203,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")                                               \
@@ -258,7 +257,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
@@ -12,7 +12,6 @@
 #include <sys/stat.h>  // S_IFDIR
 
 #include <algorithm>
-#include <climits>  // PATH_MAX
 
 namespace node {
 namespace loader {
@@ -908,6 +907,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 +971,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 +1037,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 +1146,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;
     }
   }
diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs
@@ -125,6 +125,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
diff --git a/test/fixtures/node_modules/pkgexports/package.json b/test/fixtures/node_modules/pkgexports/package.json
