modules: remove file extension and directory resolving for esm

this build on top of the limitations of the package name map proposal. It removes file extensions and directory resolution in the resolver. This is only currently limited for local development. When resolving dependencies file extension and directory resolution will still work. Refs: https://github.com/domenic/package-name-maps
nodejs · Sep 12, 2018 · 2ace497 · 2ace497
1 parent 0ade10d
commit 2ace497
Show file tree

Hide file tree

Showing 37 changed files with 106 additions and 88 deletions.
diff --git a/doc/api/esm.md b/doc/api/esm.md
@@ -54,6 +54,14 @@ property:
 
 ## Notable differences between `import` and `require`
 
+### 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
@@ -78,8 +86,8 @@ 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.

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -42,6 +42,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,
@@ -733,7 +734,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
@@ -552,6 +552,12 @@ enum ResolveExtensionsOptions {
   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) {
@@ -589,10 +595,20 @@ Maybe<URL> ResolveMain(Environment* env, const URL& search) {
       pjson.has_main == HasMain::No) {
     return Nothing<URL>();
   }
-  if (!ShouldBeTreatedAsRelativeOrAbsolutePath(pjson.main)) {
-    return Resolve(env, "./" + pjson.main, search, IgnoreMain);
+
+  URL resolved;
+  if (ShouldBeTreatedAsRelativeOrAbsolutePath(pjson.main)) {
+    resolved = URL(pjson.main, search);
+  } else {
+    resolved = URL("./" + pjson.main, search);
+  }
+  Maybe<URL> file = ResolveExtensions<TRY_EXACT_NAME>(resolved);
+  if (!file.IsNothing())
+    return file;
+  if (pjson.main.back() != '/') {
+    resolved = URL(pjson.main + "/", search);
   }
-  return Resolve(env, pjson.main, search, IgnoreMain);
+  return ResolveIndex(resolved);
 }
 
 Maybe<URL> ResolveModule(Environment* env,
@@ -603,7 +619,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 =
@@ -623,23 +639,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
@@ -654,13 +658,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,6 +1,7 @@
 // Flags: --experimental-modules
-import '../common';
+/* eslint-disable node-core/required-modules */
+import '../common/index.mjs';
 
 // Assert we can import files with `%` in their pathname.
 
-import '../fixtures/es-modules/test-esm-double-encoding-native%2520.js';
+import '../fixtures/es-modules/test-esm-double-encoding-native%2520.mjs';
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-json.mjs b/test/es-module/test-esm-json.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 json from '../fixtures/es-modules/json.json';

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,5 @@
 // Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-invalid-format.mjs
-import { expectsError, mustCall } from '../common';
+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,5 @@
 // Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-invalid-url.mjs
-import { expectsError, mustCall } from '../common';
+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,6 @@
 // Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/missing-dynamic-instantiate-hook.mjs
 
-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
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 '../common';
+/* eslint-disable node-core/required-modules */
+import '../common/index.mjs';
 import '../fixtures/es-module-require-cache/preload.js';
 import '../fixtures/es-module-require-cache/counter.js';
 import assert from 'assert';

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

diff --git a/test/es-module/test-esm-shebang.mjs b/test/es-module/test-esm-shebang.mjs
@@ -1,6 +1,7 @@
 #! }]) // isn't js
 // Flags: --experimental-modules
-import '../common';
+/* eslint-disable node-core/required-modules */
+import '../common/index.mjs';
 
 const isJs = true;
 export default isJs;
diff --git a/test/es-module/test-esm-snapshot.mjs b/test/es-module/test-esm-snapshot.mjs
@@ -1,7 +1,8 @@
 // Flags: --experimental-modules
-import '../common';
-import '../fixtures/es-modules/esm-snapshot-mutator';
-import one from '../fixtures/es-modules/esm-snapshot';
+/* eslint-disable node-core/required-modules */
+import '../common/index.mjs';
+import '../fixtures/es-modules/esm-snapshot-mutator.js';
+import one from '../fixtures/es-modules/esm-snapshot.js';
 import assert from 'assert';
 
 assert.strictEqual(one, 1);
diff --git a/test/es-module/test-esm-symlink-main.js b/test/es-module/test-esm-symlink-main.js
@@ -9,7 +9,7 @@ const fs = require('fs');
 tmpdir.refresh();
 
 const realPath = path.resolve(__dirname, '../fixtures/es-modules/symlink.mjs');
-const symlinkPath = path.resolve(tmpdir.path, 'symlink.js');
+const symlinkPath = path.resolve(tmpdir.path, 'symlink.mjs');
 
 try {
   fs.symlinkSync(realPath, symlinkPath);

diff --git a/test/es-module/test-esm-symlink.js b/test/es-module/test-esm-symlink.js
@@ -12,8 +12,8 @@ const tmpDir = tmpdir.path;
 
 const entry = path.join(tmpDir, 'entry.mjs');
 const real = path.join(tmpDir, 'index.mjs');
-const link_absolute_path = path.join(tmpDir, 'absolute');
-const link_relative_path = path.join(tmpDir, 'relative');
+const link_absolute_path = path.join(tmpDir, 'absolute.mjs');
+const link_relative_path = path.join(tmpDir, 'relative.mjs');
 const link_ignore_extension = path.join(tmpDir,
                                         'ignore_extension.json');
 const link_directory = path.join(tmpDir, 'directory');
@@ -22,15 +22,13 @@ fs.writeFileSync(real, 'export default [];');
 fs.writeFileSync(entry, `
 import assert from 'assert';
 import real from './index.mjs';
-import absolute from './absolute';
-import relative from './relative';
+import absolute from './absolute.mjs';
+import relative from './relative.mjs';
 import ignoreExtension from './ignore_extension.json';
-import directory from './directory';
 
 assert.strictEqual(absolute, real);
 assert.strictEqual(relative, real);
 assert.strictEqual(ignoreExtension, real);
-assert.strictEqual(directory, real);
 `);
 
 try {

diff --git a/test/es-module/test-esm-throw-undefined.mjs b/test/es-module/test-esm-throw-undefined.mjs
@@ -1,11 +1,11 @@
 // Flags: --experimental-modules
-import '../common';
+import '../common/index.mjs';
 import assert from 'assert';
 
 async function doTest() {
   await assert.rejects(
     async () => {
-      await import('../fixtures/es-module-loaders/throw-undefined');
+      await import('../fixtures/es-module-loaders/throw-undefined.mjs');
     },
     (e) => e === undefined
   );

diff --git a/test/fixtures/es-module-loaders/syntax-error-import.mjs b/test/fixtures/es-module-loaders/syntax-error-import.mjs
@@ -1 +1 @@
-import { foo, notfound } from './module-named-exports';
+import { foo, notfound } from './module-named-exports.mjs';
diff --git a/test/fixtures/es-modules/loop.mjs b/test/fixtures/es-modules/loop.mjs
@@ -1,4 +1,4 @@
-import { message } from './message';
+import { message } from './message.mjs';
 
 var t = 1;
 var k = 1;

diff --git a/test/fixtures/es-modules/pjson-main/main.js b/test/fixtures/es-modules/pjson-main/main.js
diff --git a/test/fixtures/es-modules/pjson-main/package.json b/test/fixtures/es-modules/pjson-main/package.json
diff --git a/test/fixtures/es-modules/test-esm-double-encoding-native%20.mjs b/test/fixtures/es-modules/test-esm-double-encoding-native%20.mjs
@@ -0,0 +1,6 @@
+'use strict';
+
+// Trivial test to assert we can load files with `%` in their pathname.
+// Imported by `test-esm-double-encoding.mjs`.
+
+export default 42;
diff --git a/test/message/esm_display_syntax_error_import.mjs b/test/message/esm_display_syntax_error_import.mjs
@@ -1,7 +1,7 @@
 // Flags:  --experimental-modules
-/* eslint-disable no-unused-vars */
-import '../common';
+/* eslint-disable no-unused-vars, node-core/required-modules */
+import '../common/index.mjs';
 import {
   foo,
   notfound
-} from '../fixtures/es-module-loaders/module-named-exports';
+} from '../fixtures/es-module-loaders/module-named-exports.mjs';
diff --git a/test/message/esm_display_syntax_error_import.out b/test/message/esm_display_syntax_error_import.out
@@ -2,5 +2,5 @@
 file:///*/test/message/esm_display_syntax_error_import.mjs:6
   notfound
   ^^^^^^^^
-SyntaxError: The requested module '../fixtures/es-module-loaders/module-named-exports' does not provide an export named 'notfound'
+SyntaxError: The requested module '../fixtures/es-module-loaders/module-named-exports.mjs' does not provide an export named 'notfound'
     at ModuleJob._instantiate (internal/modules/esm/module_job.js:*:*)