Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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
  • Loading branch information
MylesBorins committed Aug 9, 2018
commit b0e1d7ed3fdceae8513214d88370da9abcacfae5
12 changes: 10 additions & 2 deletions doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
7 changes: 6 additions & 1 deletion lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -728,7 +729,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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if we could move this filtering logic into the resolver itself still, to ensure a single code path for resolution.

if (asyncESM === undefined) lazyLoadESM();
asyncESM.loaderPromise.then((loader) => {
return loader.import(getURLFromFilePath(process.argv[1]).pathname);
Expand Down
41 changes: 30 additions & 11 deletions src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -590,9 +596,9 @@ Maybe<URL> ResolveMain(Environment* env, const URL& search) {
return Nothing<URL>();
}
if (!ShouldBeTreatedAsRelativeOrAbsolutePath(pjson.main)) {
return Resolve(env, "./" + pjson.main, search, IgnoreMain);
return ResolveRecurse(env, "./" + pjson.main, search, IgnoreMain);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably be simplified to a ResolveRelativeWithExtensions, that doesn't need to do the absolute checks and assumes its input is a relative URL.

}
return Resolve(env, pjson.main, search, IgnoreMain);
return ResolveRecurse(env, pjson.main, search, IgnoreMain);
}

Maybe<URL> ResolveModule(Environment* env,
Expand All @@ -603,7 +609,7 @@ Maybe<URL> ResolveModule(Environment* env,
do {
dir = parent;
Maybe<URL> check =
Resolve(env, "./node_modules/" + specifier, dir, CheckMain);
ResolveRecurse(env, "./node_modules/" + specifier, dir, CheckMain);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simplicity, this can stay as just Resolve.

if (!check.IsNothing()) {
const size_t limit = specifier.find('/');
const size_t spec_len =
Expand Down Expand Up @@ -636,10 +642,27 @@ Maybe<URL> ResolveDirectory(Environment* env,

} // anonymous namespace

Maybe<URL> Resolve(Environment* env,
Maybe<URL> ResolveRecurse(Environment* env,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was done to make a separate code path for resolving dependencies than local modules. Not 100% happy with the current implementation... but there is definitely a difference in the algorithm now (still need to resolve main for modules).

Open to suggestions on better ways to do this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is renamed to ResolveRelativeWithExtensions, and used only by main lookups, then the ShouldBeTreatedAsRelativeOrAbsolutePath check can be skipped.

const std::string& specifier,
const URL& base,
PackageMainCheck check_pjson_main) {
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);
} else {
return ResolveModule(env, specifier, base);
}
}

Maybe<URL> Resolve(Environment* env,
const std::string& specifier,
const URL& base) {
URL pure_url(specifier);
if (!(pure_url.flags() & URL_FLAGS_FAILED)) {
// just check existence, without altering
Expand All @@ -654,13 +677,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's all @zenparsing

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, this C++ code is a pleasure to work with 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the implementation work of @bmeck, with many various refactoring contribs.

return Nothing<URL>();
} else {
return ResolveModule(env, specifier, base);
}
Expand Down
6 changes: 5 additions & 1 deletion src/module_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,15 @@ enum PackageMainCheck : bool {
IgnoreMain = false
};

v8::Maybe<url::URL> Resolve(Environment* env,
v8::Maybe<url::URL> ResolveRecurse(Environment* env,
const std::string& specifier,
const url::URL& base,
PackageMainCheck read_pkg_json = CheckMain);

v8::Maybe<url::URL> Resolve(Environment* env,
const std::string& specifier,
const url::URL& base);

class ModuleWrap : public BaseObject {
public:
static const std::string EXTENSIONS[];
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-basic-imports.mjs
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
5 changes: 3 additions & 2 deletions test/es-module/test-esm-cyclic-dynamic-import.mjs
Original file line number Diff line number Diff line change
@@ -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');
5 changes: 3 additions & 2 deletions test/es-module/test-esm-double-encoding.mjs
Original file line number Diff line number Diff line change
@@ -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';
3 changes: 2 additions & 1 deletion test/es-module/test-esm-encoded-path.mjs
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-forbidden-globals.mjs
Original file line number Diff line number Diff line change
@@ -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') {
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-import-meta.mjs
Original file line number Diff line number Diff line change
@@ -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);
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-json.mjs
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-live-binding.mjs
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
2 changes: 1 addition & 1 deletion test/es-module/test-esm-loader-invalid-format.mjs
Original file line number Diff line number Diff line change
@@ -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')
Expand Down
2 changes: 1 addition & 1 deletion test/es-module/test-esm-loader-invalid-url.mjs
Original file line number Diff line number Diff line change
@@ -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')
Expand Down
Original file line number Diff line number Diff line change
@@ -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.mjs';

import('test').catch(expectsError({
code: 'ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK',
Expand Down
6 changes: 0 additions & 6 deletions test/es-module/test-esm-main-lookup.mjs

This file was deleted.

3 changes: 2 additions & 1 deletion test/es-module/test-esm-named-exports.mjs
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
4 changes: 3 additions & 1 deletion test/es-module/test-esm-namespace.mjs
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
2 changes: 1 addition & 1 deletion test/es-module/test-esm-preserve-symlinks-not-found.mjs
Original file line number Diff line number Diff line change
@@ -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';
3 changes: 2 additions & 1 deletion test/es-module/test-esm-require-cache.mjs
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-shared-loader-dep.mjs
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-shebang.mjs
Original file line number Diff line number Diff line change
@@ -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;
7 changes: 4 additions & 3 deletions test/es-module/test-esm-snapshot.mjs
Original file line number Diff line number Diff line change
@@ -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);
2 changes: 1 addition & 1 deletion test/es-module/test-esm-symlink-main.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
10 changes: 4 additions & 6 deletions test/es-module/test-esm-symlink.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion test/es-module/test-esm-throw-undefined.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ 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
);
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/es-module-loaders/syntax-error-import.mjs
Original file line number Diff line number Diff line change
@@ -1 +1 @@
import { foo, notfound } from './module-named-exports';
import { foo, notfound } from './module-named-exports.mjs';
2 changes: 1 addition & 1 deletion test/fixtures/es-modules/loop.mjs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { message } from './message';
import { message } from './message.mjs';

var t = 1;
var k = 1;
Expand Down
1 change: 0 additions & 1 deletion test/fixtures/es-modules/pjson-main/main.js

This file was deleted.

3 changes: 0 additions & 3 deletions test/fixtures/es-modules/pjson-main/package.json

This file was deleted.

Original file line number Diff line number Diff line change
@@ -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;
6 changes: 3 additions & 3 deletions test/message/esm_display_syntax_error_import.mjs
Original file line number Diff line number Diff line change
@@ -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';
2 changes: 1 addition & 1 deletion test/message/esm_display_syntax_error_import.out
Original file line number Diff line number Diff line change
Expand Up @@ -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:*:*)
5 changes: 3 additions & 2 deletions test/message/esm_display_syntax_error_import_module.mjs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// Flags: --experimental-modules
import '../common';
import '../fixtures/es-module-loaders/syntax-error-import';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';
import '../fixtures/es-module-loaders/syntax-error-import.mjs';
Loading