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
module: esm mode flag
This implements a package.json "mode": "esm" module boundary. When
loading a module from a package with this esm mode, ".js" files are
loaded as es modules.

In addition package.json caching is implemented in the module lookup
process, and package.json boundaries are only checked when necessary.
  • Loading branch information
guybedford committed Oct 2, 2018
commit c53a95888e39b6fb4cf29968997e676aed9fbe15
7 changes: 4 additions & 3 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ module.exports = {
overrides: [
{
files: [
'doc/api/esm.md',
'*.mjs',
'test/es-module/test-esm-example-loader.js',
"doc/api/esm.md",
"*.mjs",
"test/es-module/esm-dep.js",
"test/es-module/test-esm-example-loader.js"
],
parserOptions: { sourceType: 'module' },
},
Expand Down
28 changes: 20 additions & 8 deletions lib/internal/modules/esm/default_resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ const { pathToFileURL, fileURLToPath } = require('internal/url');

const realpathCache = new Map();

function search(target, base) {
function search(target, base, checkPjsonMode) {
if (base === undefined) {
// We cannot search without a base.
throw new ERR_MISSING_MODULE(target);
}
try {
return moduleWrapResolve(target, base);
return moduleWrapResolve(target, base, checkPjsonMode);
} catch (e) {
e.stack; // cause V8 to generate stack before rethrow
let error = e;
Expand All @@ -43,14 +43,22 @@ function search(target, base) {
}
}

const extensionFormatMap = {
const extensionFormatCjs = {
'__proto__': null,
'.mjs': 'esm',
'.json': 'json',
'.node': 'addon',
'.js': 'cjs'
};

const extensionFormatEsm = {
'__proto__': null,
'.mjs': 'esm',
'.json': 'json',
'.node': 'addon',
'.js': 'esm'
};

function resolve(specifier, parentURL) {
if (NativeModule.nonInternalExists(specifier)) {
return {
Expand All @@ -59,10 +67,14 @@ function resolve(specifier, parentURL) {
};
}

let url;
let url, esm;
try {
url = search(specifier,
parentURL || pathToFileURL(`${process.cwd()}/`).href);
// "esm" indicates if the package boundary is "mode": "esm"
// where this is only checked for ambiguous extensions
({ url, esm } =
search(specifier,
parentURL || pathToFileURL(`${process.cwd()}/`).href,
false));
} catch (e) {
if (typeof e.message === 'string' &&
StringStartsWith(e.message, 'Cannot find module'))
Expand All @@ -84,9 +96,9 @@ function resolve(specifier, parentURL) {

const ext = extname(url.pathname);

let format = extensionFormatMap[ext];
let format = (esm ? extensionFormatEsm : extensionFormatCjs)[ext];
if (!format) {
if (isMain)
if (!esm && isMain)
format = 'cjs';
else
throw new ERR_UNKNOWN_FILE_EXTENSION(url.pathname);
Expand Down
13 changes: 9 additions & 4 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,13 @@ struct PackageConfig {
enum class Exists { Yes, No };
enum class IsValid { Yes, No };
enum class HasMain { Yes, No };
enum class PackageMode { NONE, CJS, ESM };

Exists exists;
IsValid is_valid;
HasMain has_main;
std::string main;
const Exists exists;
const IsValid is_valid;
const HasMain has_main;
const std::string main;
const PackageMode mode;
};
} // namespace loader

Expand Down Expand Up @@ -161,6 +163,8 @@ struct PackageConfig {
V(env_var_settings_string, "envVarSettings") \
V(errno_string, "errno") \
V(error_string, "error") \
V(esm_string, "esm") \
V(exiting_string, "_exiting") \
V(exit_code_string, "exitCode") \
V(expire_string, "expire") \
V(exponent_string, "exponent") \
Expand Down Expand Up @@ -202,6 +206,7 @@ struct PackageConfig {
V(message_port_string, "messagePort") \
V(message_port_constructor_string, "MessagePort") \
V(minttl_string, "minttl") \
V(mode_string, "mode") \
V(modulus_string, "modulus") \
V(name_string, "name") \
V(netmask_string, "netmask") \
Expand Down
90 changes: 83 additions & 7 deletions src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ const PackageConfig& GetPackageConfig(Environment* env,
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, "" });
PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "", NONE });
return entry.first->second;
}

Expand All @@ -520,7 +520,7 @@ const PackageConfig& GetPackageConfig(Environment* env,
v8::NewStringType::kNormal,
pkg_src.length()).ToLocal(&src)) {
auto entry = env->package_json_cache.emplace(path,
PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "" });
PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "", NONE });
return entry.first->second;
}

Expand All @@ -530,7 +530,7 @@ const PackageConfig& GetPackageConfig(Environment* env,
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, "" });
PackageConfig { Exists::Yes, IsValid::No, HasMain::No, "", NONE });
return entry.first->second;
}

Expand All @@ -543,11 +543,51 @@ const PackageConfig& GetPackageConfig(Environment* env,
main_std.assign(std::string(*main_utf8, main_utf8.length()));
}

Local<Value> pkg_mode_v;
PackageMode pkg_mode = CJS;
std::string pkg_mode_std;
if (pkg_json->Get(env->context(), env->mode_string()).ToLocal(&pkg_mode_v)) {
Utf8Value pkg_mode_utf8(isolate, pkg_mode_v);
pkg_mode_std.assign(std::string(*pkg_mode_utf8, pkg_mode_utf8.length()));
if (pkg_mode_std == "esm") {
pkg_mode = ESM;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Simpler:

if (pkg_json->Get(env->context(), env->mode_string()).ToLocal(&pkg_mode_v)) &&
    pkg_mode_v->StrictEquals(env->esm_string())) {
  pkg_mode = ESM;
}


auto entry = env->package_json_cache.emplace(path,
PackageConfig { Exists::Yes, IsValid::Yes, has_main, main_std });
PackageConfig { Exists::Yes, IsValid::Yes, has_main,
main_std, pkg_mode });
return entry.first->second;
}

Copy link
Member

Choose a reason for hiding this comment

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

If this C++ json reader/parser is faster than JS would it be possible in a follow up PR to just use it for reading of package.json's or at least cached the parsed data here to avoid a subsequent load/parse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the goal is to unify on a single cache with further work.

PackageMode GetPackageMode(Environment* env, const URL& search) {
URL pjsonPath("package.json", &search);
Copy link
Member

Choose a reason for hiding this comment

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

Style: pjson_path (i.e. snake_case, not camelCase.). Ditto on line 571, 581, 754, etc.

while (true) {
const PackageConfig& pkg_json =
GetPackageConfig(env, pjsonPath.ToFilePath());
if (pkg_json.exists == Exists::Yes) {
return pkg_json.mode;
}
URL lastPjsonPath = pjsonPath;
pjsonPath = URL("../package.json", pjsonPath);
if (pjsonPath.path() == lastPjsonPath.path()) {
return CJS;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

What guarantees that this loop terminates? It's not evident to me. A comment or a check is probably in order.

}

void SetPackageMode(Environment* env, const URL& search,
PackageMode pkg_mode) {
std::string pjsonPathStr = URL("package.json", &search).ToFilePath();
const PackageConfig& pkg_json = GetPackageConfig(env, pjsonPathStr);
if (pkg_json.mode != pkg_mode) {
env->package_json_cache.erase(env->package_json_cache.find(pjsonPathStr));
env->package_json_cache.emplace(pjsonPathStr,
PackageConfig { pkg_json.exists, pkg_json.is_valid,
pkg_json.has_main, pkg_json.main, pkg_mode });
}
}

enum ResolveExtensionsOptions {
TRY_EXACT_NAME,
ONLY_VIA_EXTENSIONS
Expand Down Expand Up @@ -670,8 +710,8 @@ Maybe<URL> Resolve(Environment* env,
void ModuleWrap::Resolve(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

// module.resolve(specifier, url)
CHECK_EQ(args.Length(), 2);
// module.resolve(specifier, url, setPackageEsmMode)
CHECK_EQ(args.Length(), 3);

CHECK(args[0]->IsString());
Utf8Value specifier_utf8(env->isolate(), args[0]);
Expand All @@ -686,13 +726,49 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo<Value>& args) {
env, "second argument is not a URL string");
}

if (!args[2]->IsBoolean()) {
env->ThrowError("third argument is not a boolean");
return;
}

Maybe<URL> result = node::loader::Resolve(env, specifier_std, url);
if (result.IsNothing() || (result.FromJust().flags() & URL_FLAGS_FAILED)) {
std::string msg = "Cannot find module " + specifier_std;
return node::THROW_ERR_MISSING_MODULE(env, msg.c_str());
}

args.GetReturnValue().Set(result.FromJust().ToObject(env));
bool esmPackage = false;
bool set_package_esm_mode = args[2]->ToBoolean().As<v8::Boolean>()->Value();
Copy link
Member

Choose a reason for hiding this comment

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

args[2]->IsTrue() (and make set_package_esm_mode const or simply drop it.)

if (set_package_esm_mode) {
esmPackage = true;
SetPackageMode(env, result.FromJust(), ESM);
} else {
std::string filePath = result.FromJust().ToFilePath();
// check the package esm mode for ambiguous extensions
Copy link
Member

Choose a reason for hiding this comment

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

Style: can you capitalize and punctuate the comment?

if (filePath.substr(filePath.length() - 4, 4) != ".mjs" &&
filePath.substr(filePath.length() - 5, 5) != ".json" &&
filePath.substr(filePath.length() - 5, 5) != ".node") {
Copy link
Member

Choose a reason for hiding this comment

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

Unsafe when .length() is < than the value subtracted from it.

if (GetPackageMode(env, result.FromJust()) == ESM) {
esmPackage = true;
}
}
}

Local<Object> resolved = Object::New(env->isolate());

(void)resolved->DefineOwnProperty(
Copy link
Member

Choose a reason for hiding this comment

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

Don't cast to void, call .FromJust().

env->context(),
env->esm_string(),
v8::Boolean::New(env->isolate(), esmPackage),
v8::ReadOnly);

(void)resolved->DefineOwnProperty(
env->context(),
env->url_string(),
result.FromJust().ToObject(env),
v8::ReadOnly);

args.GetReturnValue().Set(resolved);
}

static MaybeLocal<Promise> ImportModuleDynamically(
Expand Down
6 changes: 6 additions & 0 deletions test/es-module/test-esm-node-modules.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Flags: --experimental-modules
import '../common';
import assert from 'assert';
import { x } from '../fixtures/es-modules/node_lookups/module.mjs';

assert.deepStrictEqual(x, 42);
6 changes: 6 additions & 0 deletions test/es-module/test-pjson-cjs-esm-nested.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Flags: --experimental-modules
/* eslint-disable required-modules */
import m from '../fixtures/es-modules/esm-cjs-nested/module';
import assert from 'assert';

assert.strictEqual(m, 'cjs');
6 changes: 6 additions & 0 deletions test/es-module/test-pjson-esm-mode-invalid.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Flags: --experimental-modules
/* eslint-disable required-modules */
import m from '../fixtures/es-modules/esm-invalid/module';
import assert from 'assert';

assert.strictEqual(m, 'cjs');
6 changes: 6 additions & 0 deletions test/es-module/test-pjson-esm-mode-submodule.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Flags: --experimental-modules
/* eslint-disable required-modules */
import m from '../fixtures/es-modules/esm/sub/dir/module';
import assert from 'assert';

assert.strictEqual(m, 'esm submodule');
6 changes: 6 additions & 0 deletions test/es-module/test-pjson-esm-mode.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Flags: --experimental-modules
/* eslint-disable required-modules */
import m from '../fixtures/es-modules/esm/module';
import assert from 'assert';

assert.strictEqual(m, 'esm');
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = 'cjs';
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
1 change: 1 addition & 0 deletions test/fixtures/es-modules/esm-cjs-nested/module.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './cjs-nested/module.js';
3 changes: 3 additions & 0 deletions test/fixtures/es-modules/esm-cjs-nested/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"mode": "esm"
}
1 change: 1 addition & 0 deletions test/fixtures/es-modules/esm-invalid/module.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = 'cjs';
3 changes: 3 additions & 0 deletions test/fixtures/es-modules/esm-invalid/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"mode": 1
}
1 change: 1 addition & 0 deletions test/fixtures/es-modules/esm/module.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default 'esm';
3 changes: 3 additions & 0 deletions test/fixtures/es-modules/esm/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"mode": "esm"
}
1 change: 1 addition & 0 deletions test/fixtures/es-modules/esm/sub/dir/module.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default 'esm submodule';
1 change: 1 addition & 0 deletions test/fixtures/es-modules/node_lookups/module.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default as x } from 'cjs';

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.