esm: remove node specifier resolution algorithm

Refs: nodejs/modules#180
nodejs · Oct 2, 2018 · de223bb · de223bb
1 parent 3c75a81
commit de223bb
Show file tree

Hide file tree

Showing 37 changed files with 120 additions and 162 deletions.
diff --git a/doc/api/esm.md b/doc/api/esm.md
@@ -58,6 +58,14 @@ property:
 
 ESM must have the `.mjs` extension.
 
+### Mandatory file extensions
+
+You must provide a file extension when using the `import` keyword.
+
+### No importing directories
+
+There is no support for importing directories.
+
 ### No NODE_PATH
 
 `NODE_PATH` is not part of resolving `import` specifiers. Please use symlinks
@@ -82,21 +90,15 @@ Modules will be loaded multiple times if the `import` specifier used to resolve
 them have a different query or fragment.
 
 ```js
-import './foo?query=1'; // loads ./foo with query of "?query=1"
-import './foo?query=2'; // loads ./foo with query of "?query=2"
+import './foo.mjs?query=1'; // loads ./foo.mjs with query of "?query=1"
+import './foo.mjs?query=2'; // loads ./foo.mjs with query of "?query=2"
 ```
 
 For now, only modules using the `file:` protocol can be loaded.
 
-## Interop with existing modules
+## CommonJS,  JSON, and Native Modules
 
-CommonJS and C++ modules can be used with `import`.
-
-Modules loaded this way will only be loaded once, even if their query
-or fragment string differs between `import` statements.
-
-When loaded via `import` these modules will provide a single `default` export
-representing the value of `module.exports` at the time they finished evaluating.
+CommonJS, JSON, and Native modules can be used with [`module.createRequireFromPath()`][`module.createRequireFromPath()`].
 
 ```js
 // cjs.js
@@ -112,6 +114,8 @@ const cjs = require('./cjs');
 foo.cjs === 'cjs'; // true
 ```
 
+## Builtin modules
+
 Builtin modules will provide named exports of their public API, as well as a
 default export which can be used for, among other things, modifying the named
 exports. Named exports of builtin modules are updated when the corresponding
@@ -261,3 +265,4 @@ in the import tree.
 
 [Node.js EP for ES Modules]: https://github.com/nodejs/node-eps/blob/master/002-es-modules.md
 [dynamic instantiate hook]: #esm_dynamic_instantiate_hook
+[`module.createRequireFromPath()`]: modules.html#modules_module_createrequirefrompath_filename
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -43,6 +43,7 @@ const {
 const preserveSymlinks = !!process.binding('config').preserveSymlinks;
 const preserveSymlinksMain = !!process.binding('config').preserveSymlinksMain;
 const experimentalModules = !!process.binding('config').experimentalModules;
+const hasLoader = !!process.binding('config').userLoader;
 
 const {
   ERR_INVALID_ARG_TYPE,
@@ -742,7 +743,11 @@ if (experimentalModules) {
 // bootstrap main module.
 Module.runMain = function() {
   // Load the main module--the command line argument.
-  if (experimentalModules) {
+  const base = path.basename(process.argv[1]);
+  const ext = path.extname(base);
+  const isESM = ext === '.mjs';
+
+  if (experimentalModules && (isESM || hasLoader)) {
     if (asyncESM === undefined) lazyLoadESM();
     asyncESM.loaderPromise.then((loader) => {
       return loader.import(pathToFileURL(process.argv[1]).pathname);

diff --git a/src/module_wrap.cc b/src/module_wrap.cc
@@ -26,7 +26,6 @@ using v8::HandleScope;
 using v8::Integer;
 using v8::IntegrityLevel;
 using v8::Isolate;
-using v8::JSON;
 using v8::Just;
 using v8::Local;
 using v8::Maybe;
@@ -489,70 +488,17 @@ Maybe<uv_file> CheckFile(const std::string& path,
   return Just(fd);
 }
 
-using Exists = PackageConfig::Exists;
-using IsValid = PackageConfig::IsValid;
-using HasMain = PackageConfig::HasMain;
-
-const PackageConfig& GetPackageConfig(Environment* env,
-                                      const std::string& path) {
-  auto existing = env->package_json_cache.find(path);
-  if (existing != env->package_json_cache.end()) {
-    return existing->second;
-  }
-  Maybe<uv_file> check = CheckFile(path, LEAVE_OPEN_AFTER_CHECK);
-  if (check.IsNothing()) {
-    auto entry = env->package_json_cache.emplace(path,
-        PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "" });
-    return entry.first->second;
-  }
-
-  Isolate* isolate = env->isolate();
-  v8::HandleScope handle_scope(isolate);
-
-  std::string pkg_src = ReadFile(check.FromJust());
-  uv_fs_t fs_req;
-  CHECK_EQ(0, uv_fs_close(nullptr, &fs_req, check.FromJust(), nullptr));
-  uv_fs_req_cleanup(&fs_req);
-
-  Local<String> src;
-  if (!String::NewFromUtf8(isolate,
-                           pkg_src.c_str(),
-                           v8::NewStringType::kNormal,
-                           pkg_src.length()).ToLocal(&src)) {
-    auto entry = env->package_json_cache.emplace(path,
-        PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "" });
-    return entry.first->second;
-  }
-
-  Local<Value> pkg_json_v;
-  Local<Object> pkg_json;
-
-  if (!JSON::Parse(env->context(), src).ToLocal(&pkg_json_v) ||
-      !pkg_json_v->ToObject(env->context()).ToLocal(&pkg_json)) {
-    auto entry = env->package_json_cache.emplace(path,
-        PackageConfig { Exists::Yes, IsValid::No, HasMain::No, "" });
-    return entry.first->second;
-  }
-
-  Local<Value> pkg_main;
-  HasMain has_main = HasMain::No;
-  std::string main_std;
-  if (pkg_json->Get(env->context(), env->main_string()).ToLocal(&pkg_main)) {
-    has_main = HasMain::Yes;
-    Utf8Value main_utf8(isolate, pkg_main);
-    main_std.assign(std::string(*main_utf8, main_utf8.length()));
-  }
-
-  auto entry = env->package_json_cache.emplace(path,
-      PackageConfig { Exists::Yes, IsValid::Yes, has_main, main_std });
-  return entry.first->second;
-}
-
 enum ResolveExtensionsOptions {
   TRY_EXACT_NAME,
   ONLY_VIA_EXTENSIONS
 };
 
+inline bool ResolvesToFile(const URL& search) {
+  std::string filePath = search.ToFilePath();
+  Maybe<uv_file> check = CheckFile(filePath);
+  return !check.IsNothing();
+}
+
 template <ResolveExtensionsOptions options>
 Maybe<URL> ResolveExtensions(const URL& search) {
   if (options == TRY_EXACT_NAME) {
@@ -578,24 +524,6 @@ inline Maybe<URL> ResolveIndex(const URL& search) {
   return ResolveExtensions<ONLY_VIA_EXTENSIONS>(URL("index", search));
 }
 
-Maybe<URL> ResolveMain(Environment* env, const URL& search) {
-  URL pkg("package.json", &search);
-
-  const PackageConfig& pjson =
-      GetPackageConfig(env, pkg.ToFilePath());
-  // Note invalid package.json should throw in resolver
-  // currently we silently ignore which is incorrect
-  if (pjson.exists == Exists::No ||
-      pjson.is_valid == IsValid::No ||
-      pjson.has_main == HasMain::No) {
-    return Nothing<URL>();
-  }
-  if (!ShouldBeTreatedAsRelativeOrAbsolutePath(pjson.main)) {
-    return Resolve(env, "./" + pjson.main, search, IgnoreMain);
-  }
-  return Resolve(env, pjson.main, search, IgnoreMain);
-}
-
 Maybe<URL> ResolveModule(Environment* env,
                          const std::string& specifier,
                          const URL& base) {
@@ -604,7 +532,7 @@ Maybe<URL> ResolveModule(Environment* env,
   do {
     dir = parent;
     Maybe<URL> check =
-        Resolve(env, "./node_modules/" + specifier, dir, CheckMain);
+        Resolve(env, "./node_modules/" + specifier, dir);
     if (!check.IsNothing()) {
       const size_t limit = specifier.find('/');
       const size_t spec_len =
@@ -624,23 +552,11 @@ Maybe<URL> ResolveModule(Environment* env,
   return Nothing<URL>();
 }
 
-Maybe<URL> ResolveDirectory(Environment* env,
-                            const URL& search,
-                            PackageMainCheck check_pjson_main) {
-  if (check_pjson_main) {
-    Maybe<URL> main = ResolveMain(env, search);
-    if (!main.IsNothing())
-      return main;
-  }
-  return ResolveIndex(search);
-}
-
 }  // anonymous namespace
 
 Maybe<URL> Resolve(Environment* env,
                    const std::string& specifier,
-                   const URL& base,
-                   PackageMainCheck check_pjson_main) {
+                   const URL& base) {
   URL pure_url(specifier);
   if (!(pure_url.flags() & URL_FLAGS_FAILED)) {
     // just check existence, without altering
@@ -655,13 +571,9 @@ Maybe<URL> Resolve(Environment* env,
   }
   if (ShouldBeTreatedAsRelativeOrAbsolutePath(specifier)) {
     URL resolved(specifier, base);
-    Maybe<URL> file = ResolveExtensions<TRY_EXACT_NAME>(resolved);
-    if (!file.IsNothing())
-      return file;
-    if (specifier.back() != '/') {
-      resolved = URL(specifier + "/", base);
-    }
-    return ResolveDirectory(env, resolved, check_pjson_main);
+    if (ResolvesToFile(resolved))
+      return Just(resolved);
+    return Nothing<URL>();
   } else {
     return ResolveModule(env, specifier, base);
   }

diff --git a/src/module_wrap.h b/src/module_wrap.h
@@ -12,15 +12,9 @@
 namespace node {
 namespace loader {
 
-enum PackageMainCheck : bool {
-    CheckMain = true,
-    IgnoreMain = false
-};
-
 v8::Maybe<url::URL> Resolve(Environment* env,
                             const std::string& specifier,
-                            const url::URL& base,
-                            PackageMainCheck read_pkg_json = CheckMain);
+                            const url::URL& base);
 
 class ModuleWrap : public BaseObject {
  public:

diff --git a/test/es-module/test-esm-basic-imports.mjs b/test/es-module/test-esm-basic-imports.mjs
@@ -1,5 +1,6 @@
 // Flags: --experimental-modules
-import '../common';
+/* eslint-disable node-core/required-modules */
+import '../common/index.mjs';
 import assert from 'assert';
 import ok from '../fixtures/es-modules/test-esm-ok.mjs';
 import okShebang from './test-esm-shebang.mjs';

diff --git a/test/es-module/test-esm-cyclic-dynamic-import.mjs b/test/es-module/test-esm-cyclic-dynamic-import.mjs
@@ -1,3 +1,4 @@
 // Flags: --experimental-modules
-import '../common';
-import('./test-esm-cyclic-dynamic-import');
+/* eslint-disable node-core/required-modules */
+import '../common/index.mjs';
+import('./test-esm-cyclic-dynamic-import.mjs');
diff --git a/test/es-module/test-esm-double-encoding.mjs b/test/es-module/test-esm-double-encoding.mjs
@@ -1,5 +1,6 @@
 // Flags: --experimental-modules
-import '../common';
+/* eslint-disable node-core/required-modules */
+import '../common/index.mjs';
 
 // Assert we can import files with `%` in their pathname.
 

diff --git a/test/es-module/test-esm-encoded-path.mjs b/test/es-module/test-esm-encoded-path.mjs
@@ -1,5 +1,6 @@
 // Flags: --experimental-modules
-import '../common';
+/* eslint-disable node-core/required-modules */
+import '../common/index.mjs';
 import assert from 'assert';
 // ./test-esm-ok.mjs
 import ok from '../fixtures/es-modules/test-%65%73%6d-ok.mjs';

diff --git a/test/es-module/test-esm-forbidden-globals.mjs b/test/es-module/test-esm-forbidden-globals.mjs
@@ -1,5 +1,6 @@
 // Flags: --experimental-modules
-import '../common';
+/* eslint-disable node-core/required-modules */
+import '../common/index.mjs';
 
 // eslint-disable-next-line no-undef
 if (typeof arguments !== 'undefined') {

diff --git a/test/es-module/test-esm-import-meta.mjs b/test/es-module/test-esm-import-meta.mjs
@@ -1,6 +1,7 @@
 // Flags: --experimental-modules
+/* eslint-disable node-core/required-modules */
 
-import '../common';
+import '../common/index.mjs';
 import assert from 'assert';
 
 assert.strictEqual(Object.getPrototypeOf(import.meta), null);

diff --git a/test/es-module/test-esm-live-binding.mjs b/test/es-module/test-esm-live-binding.mjs
@@ -1,6 +1,7 @@
 // Flags: --experimental-modules
+/* eslint-disable node-core/required-modules */
 
-import '../common';
+import '../common/index.mjs';
 import assert from 'assert';
 
 import fs, { readFile, readFileSync } from 'fs';

diff --git a/test/es-module/test-esm-loader-invalid-format.mjs b/test/es-module/test-esm-loader-invalid-format.mjs
@@ -1,5 +1,6 @@
 // Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-invalid-format.mjs
-import { expectsError, mustCall } from '../common';
+/* eslint-disable node-core/required-modules */
+import { expectsError, mustCall } from '../common/index.mjs';
 import assert from 'assert';
 
 import('../fixtures/es-modules/test-esm-ok.mjs')

diff --git a/test/es-module/test-esm-loader-invalid-url.mjs b/test/es-module/test-esm-loader-invalid-url.mjs
@@ -1,5 +1,7 @@
 // Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-invalid-url.mjs
-import { expectsError, mustCall } from '../common';
+/* eslint-disable node-core/required-modules */
+
+import { expectsError, mustCall } from '../common/index.mjs';
 import assert from 'assert';
 
 import('../fixtures/es-modules/test-esm-ok.mjs')

diff --git a/test/es-module/test-esm-loader-missing-dynamic-instantiate-hook.mjs b/test/es-module/test-esm-loader-missing-dynamic-instantiate-hook.mjs
@@ -1,6 +1,7 @@
 // Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/missing-dynamic-instantiate-hook.mjs
+/* eslint-disable node-core/required-modules */
 
-import { expectsError } from '../common';
+import { expectsError } from '../common/index.mjs';
 
 import('test').catch(expectsError({
   code: 'ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK',

diff --git a/test/es-module/test-esm-main-lookup.mjs b/test/es-module/test-esm-main-lookup.mjs
@@ -1,6 +1,26 @@
 // Flags: --experimental-modules
-import '../common';
+/* eslint-disable node-core/required-modules */
+import '../common/index.mjs';
 import assert from 'assert';
-import main from '../fixtures/es-modules/pjson-main';
 
-assert.strictEqual(main, 'main');
+async function main() {
+  let mod;
+  try {
+    mod = await import('../fixtures/es-modules/pjson-main');
+  } catch (e) {
+    assert.strictEqual(e.code, 'MODULE_NOT_FOUND');
+  }
+
+  assert.strictEqual(mod, undefined);
+
+  try {
+    mod = await import('../fixtures/es-modules/pjson-main/main.mjs');
+  } catch (e) {
+    console.log(e);
+    assert.fail();
+  }
+
+  assert.strictEqual(mod.main, 'main');
+}
+
+main();
diff --git a/test/es-module/test-esm-named-exports.mjs b/test/es-module/test-esm-named-exports.mjs
@@ -1,5 +1,6 @@
 // Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/builtin-named-exports-loader.mjs
-import '../common';
+/* eslint-disable node-core/required-modules */
+import '../common/index.mjs';
 import { readFile } from 'fs';
 import assert from 'assert';
 import ok from '../fixtures/es-modules/test-esm-ok.mjs';

diff --git a/test/es-module/test-esm-namespace.mjs b/test/es-module/test-esm-namespace.mjs
@@ -1,5 +1,7 @@
 // Flags: --experimental-modules
-import '../common';
+/* eslint-disable node-core/required-modules */
+
+import '../common/index.mjs';
 import * as fs from 'fs';
 import assert from 'assert';
 import Module from 'module';

diff --git a/test/es-module/test-esm-preserve-symlinks-not-found.mjs b/test/es-module/test-esm-preserve-symlinks-not-found.mjs
@@ -1,3 +1,3 @@
 // Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/not-found-assert-loader.mjs
 /* eslint-disable node-core/required-modules */
-import './not-found';
+import './not-found.mjs';
diff --git a/test/es-module/test-esm-require-cache.mjs b/test/es-module/test-esm-require-cache.mjs
@@ -1,5 +1,6 @@
 // Flags: --experimental-modules
-import { createRequire } from '../common';
+/* eslint-disable node-core/required-modules */
+import { createRequire } from '../common/index.mjs';
 import assert from 'assert';
 //
 const require = createRequire(import.meta.url);