From c53a95888e39b6fb4cf29968997e676aed9fbe15 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 4 Feb 2018 15:29:46 +0200 Subject: [PATCH 1/5] 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. --- .eslintrc.js | 7 +- lib/internal/modules/esm/default_resolve.js | 28 ++++-- src/env.h | 13 ++- src/module_wrap.cc | 90 +++++++++++++++++-- test/es-module/test-esm-node-modules.mjs | 6 ++ test/es-module/test-pjson-cjs-esm-nested.mjs | 6 ++ .../es-module/test-pjson-esm-mode-invalid.mjs | 6 ++ .../test-pjson-esm-mode-submodule.mjs | 6 ++ test/es-module/test-pjson-esm-mode.mjs | 6 ++ .../esm-cjs-nested/cjs-nested/module.js | 1 + .../esm-cjs-nested/cjs-nested/package.json | 1 + .../es-modules/esm-cjs-nested/module.js | 1 + .../es-modules/esm-cjs-nested/package.json | 3 + .../fixtures/es-modules/esm-invalid/module.js | 1 + .../es-modules/esm-invalid/package.json | 3 + test/fixtures/es-modules/esm/module.js | 1 + test/fixtures/es-modules/esm/package.json | 3 + .../fixtures/es-modules/esm/sub/dir/module.js | 1 + .../es-modules/node_lookups/module.mjs | 1 + .../node_modules/cjs/package.json | 3 + .../node_modules/cjs/src/index.js | 1 + 21 files changed, 166 insertions(+), 22 deletions(-) create mode 100644 test/es-module/test-esm-node-modules.mjs create mode 100644 test/es-module/test-pjson-cjs-esm-nested.mjs create mode 100644 test/es-module/test-pjson-esm-mode-invalid.mjs create mode 100644 test/es-module/test-pjson-esm-mode-submodule.mjs create mode 100644 test/es-module/test-pjson-esm-mode.mjs create mode 100644 test/fixtures/es-modules/esm-cjs-nested/cjs-nested/module.js create mode 100644 test/fixtures/es-modules/esm-cjs-nested/cjs-nested/package.json create mode 100644 test/fixtures/es-modules/esm-cjs-nested/module.js create mode 100644 test/fixtures/es-modules/esm-cjs-nested/package.json create mode 100644 test/fixtures/es-modules/esm-invalid/module.js create mode 100644 test/fixtures/es-modules/esm-invalid/package.json create mode 100644 test/fixtures/es-modules/esm/module.js create mode 100644 test/fixtures/es-modules/esm/package.json create mode 100644 test/fixtures/es-modules/esm/sub/dir/module.js create mode 100644 test/fixtures/es-modules/node_lookups/module.mjs create mode 100644 test/fixtures/es-modules/node_lookups/node_modules/cjs/package.json create mode 100644 test/fixtures/es-modules/node_lookups/node_modules/cjs/src/index.js diff --git a/.eslintrc.js b/.eslintrc.js index 6a8403d3488a..e17c10f8bbf5 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -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' }, }, diff --git a/lib/internal/modules/esm/default_resolve.js b/lib/internal/modules/esm/default_resolve.js index 875c560cb150..131a72808ed6 100644 --- a/lib/internal/modules/esm/default_resolve.js +++ b/lib/internal/modules/esm/default_resolve.js @@ -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; @@ -43,7 +43,7 @@ function search(target, base) { } } -const extensionFormatMap = { +const extensionFormatCjs = { '__proto__': null, '.mjs': 'esm', '.json': 'json', @@ -51,6 +51,14 @@ const extensionFormatMap = { '.js': 'cjs' }; +const extensionFormatEsm = { + '__proto__': null, + '.mjs': 'esm', + '.json': 'json', + '.node': 'addon', + '.js': 'esm' +}; + function resolve(specifier, parentURL) { if (NativeModule.nonInternalExists(specifier)) { return { @@ -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')) @@ -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); diff --git a/src/env.h b/src/env.h index feeeda661cec..6a39ddc63a57 100644 --- a/src/env.h +++ b/src/env.h @@ -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 @@ -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") \ @@ -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") \ diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 1ef22b270d12..f0207404eaab 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -502,7 +502,7 @@ const PackageConfig& GetPackageConfig(Environment* env, Maybe 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; } @@ -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; } @@ -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; } @@ -543,11 +543,51 @@ const PackageConfig& GetPackageConfig(Environment* env, main_std.assign(std::string(*main_utf8, main_utf8.length())); } + Local 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; + } + } + 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; } +PackageMode GetPackageMode(Environment* env, const URL& search) { + URL pjsonPath("package.json", &search); + 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; + } + } +} + +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 @@ -670,8 +710,8 @@ Maybe Resolve(Environment* env, void ModuleWrap::Resolve(const FunctionCallbackInfo& 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]); @@ -686,13 +726,49 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo& args) { env, "second argument is not a URL string"); } + if (!args[2]->IsBoolean()) { + env->ThrowError("third argument is not a boolean"); + return; + } + Maybe 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()->Value(); + 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 + if (filePath.substr(filePath.length() - 4, 4) != ".mjs" && + filePath.substr(filePath.length() - 5, 5) != ".json" && + filePath.substr(filePath.length() - 5, 5) != ".node") { + if (GetPackageMode(env, result.FromJust()) == ESM) { + esmPackage = true; + } + } + } + + Local resolved = Object::New(env->isolate()); + + (void)resolved->DefineOwnProperty( + 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 ImportModuleDynamically( diff --git a/test/es-module/test-esm-node-modules.mjs b/test/es-module/test-esm-node-modules.mjs new file mode 100644 index 000000000000..59bd130f5dea --- /dev/null +++ b/test/es-module/test-esm-node-modules.mjs @@ -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); diff --git a/test/es-module/test-pjson-cjs-esm-nested.mjs b/test/es-module/test-pjson-cjs-esm-nested.mjs new file mode 100644 index 000000000000..1639abb27db5 --- /dev/null +++ b/test/es-module/test-pjson-cjs-esm-nested.mjs @@ -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'); diff --git a/test/es-module/test-pjson-esm-mode-invalid.mjs b/test/es-module/test-pjson-esm-mode-invalid.mjs new file mode 100644 index 000000000000..42e9ca3ca49d --- /dev/null +++ b/test/es-module/test-pjson-esm-mode-invalid.mjs @@ -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'); diff --git a/test/es-module/test-pjson-esm-mode-submodule.mjs b/test/es-module/test-pjson-esm-mode-submodule.mjs new file mode 100644 index 000000000000..bc855e2f24c6 --- /dev/null +++ b/test/es-module/test-pjson-esm-mode-submodule.mjs @@ -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'); diff --git a/test/es-module/test-pjson-esm-mode.mjs b/test/es-module/test-pjson-esm-mode.mjs new file mode 100644 index 000000000000..68e899eb672c --- /dev/null +++ b/test/es-module/test-pjson-esm-mode.mjs @@ -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'); diff --git a/test/fixtures/es-modules/esm-cjs-nested/cjs-nested/module.js b/test/fixtures/es-modules/esm-cjs-nested/cjs-nested/module.js new file mode 100644 index 000000000000..b2825bd3c994 --- /dev/null +++ b/test/fixtures/es-modules/esm-cjs-nested/cjs-nested/module.js @@ -0,0 +1 @@ +module.exports = 'cjs'; diff --git a/test/fixtures/es-modules/esm-cjs-nested/cjs-nested/package.json b/test/fixtures/es-modules/esm-cjs-nested/cjs-nested/package.json new file mode 100644 index 000000000000..0967ef424bce --- /dev/null +++ b/test/fixtures/es-modules/esm-cjs-nested/cjs-nested/package.json @@ -0,0 +1 @@ +{} diff --git a/test/fixtures/es-modules/esm-cjs-nested/module.js b/test/fixtures/es-modules/esm-cjs-nested/module.js new file mode 100644 index 000000000000..f419110f8068 --- /dev/null +++ b/test/fixtures/es-modules/esm-cjs-nested/module.js @@ -0,0 +1 @@ +export { default } from './cjs-nested/module.js'; diff --git a/test/fixtures/es-modules/esm-cjs-nested/package.json b/test/fixtures/es-modules/esm-cjs-nested/package.json new file mode 100644 index 000000000000..0ccace5b1841 --- /dev/null +++ b/test/fixtures/es-modules/esm-cjs-nested/package.json @@ -0,0 +1,3 @@ +{ + "mode": "esm" +} diff --git a/test/fixtures/es-modules/esm-invalid/module.js b/test/fixtures/es-modules/esm-invalid/module.js new file mode 100644 index 000000000000..b2825bd3c994 --- /dev/null +++ b/test/fixtures/es-modules/esm-invalid/module.js @@ -0,0 +1 @@ +module.exports = 'cjs'; diff --git a/test/fixtures/es-modules/esm-invalid/package.json b/test/fixtures/es-modules/esm-invalid/package.json new file mode 100644 index 000000000000..926ad27ba876 --- /dev/null +++ b/test/fixtures/es-modules/esm-invalid/package.json @@ -0,0 +1,3 @@ +{ + "mode": 1 +} diff --git a/test/fixtures/es-modules/esm/module.js b/test/fixtures/es-modules/esm/module.js new file mode 100644 index 000000000000..914e3a97d5dd --- /dev/null +++ b/test/fixtures/es-modules/esm/module.js @@ -0,0 +1 @@ +export default 'esm'; diff --git a/test/fixtures/es-modules/esm/package.json b/test/fixtures/es-modules/esm/package.json new file mode 100644 index 000000000000..0ccace5b1841 --- /dev/null +++ b/test/fixtures/es-modules/esm/package.json @@ -0,0 +1,3 @@ +{ + "mode": "esm" +} diff --git a/test/fixtures/es-modules/esm/sub/dir/module.js b/test/fixtures/es-modules/esm/sub/dir/module.js new file mode 100644 index 000000000000..60cea8ac4d68 --- /dev/null +++ b/test/fixtures/es-modules/esm/sub/dir/module.js @@ -0,0 +1 @@ +export default 'esm submodule'; diff --git a/test/fixtures/es-modules/node_lookups/module.mjs b/test/fixtures/es-modules/node_lookups/module.mjs new file mode 100644 index 000000000000..2e5b1d81045e --- /dev/null +++ b/test/fixtures/es-modules/node_lookups/module.mjs @@ -0,0 +1 @@ +export { default as x } from 'cjs'; diff --git a/test/fixtures/es-modules/node_lookups/node_modules/cjs/package.json b/test/fixtures/es-modules/node_lookups/node_modules/cjs/package.json new file mode 100644 index 000000000000..da215725c257 --- /dev/null +++ b/test/fixtures/es-modules/node_lookups/node_modules/cjs/package.json @@ -0,0 +1,3 @@ +{ + "main": "src/index.js" +} diff --git a/test/fixtures/es-modules/node_lookups/node_modules/cjs/src/index.js b/test/fixtures/es-modules/node_lookups/node_modules/cjs/src/index.js new file mode 100644 index 000000000000..888cae37af95 --- /dev/null +++ b/test/fixtures/es-modules/node_lookups/node_modules/cjs/src/index.js @@ -0,0 +1 @@ +module.exports = 42; From 89bfdfacf67de741b3385b6b374ea5c2104c089a Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 18 Feb 2018 13:13:05 +0200 Subject: [PATCH 2/5] PR feedback --- src/env.h | 2 +- src/module_wrap.cc | 58 ++++++++++++++++++++++++---------------------- 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/src/env.h b/src/env.h index 6a39ddc63a57..ae00be451c3c 100644 --- a/src/env.h +++ b/src/env.h @@ -76,7 +76,7 @@ struct PackageConfig { const IsValid is_valid; const HasMain has_main; const std::string main; - const PackageMode mode; + const PackageMode::Mode mode; }; } // namespace loader diff --git a/src/module_wrap.cc b/src/module_wrap.cc index f0207404eaab..7b49bad9a7d2 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -502,7 +502,7 @@ const PackageConfig& GetPackageConfig(Environment* env, Maybe 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, "", NONE }); + PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "", PackageMode::NONE }); return entry.first->second; } @@ -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, "", NONE }); + PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "", PackageMode::NONE }); return entry.first->second; } @@ -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, "", NONE }); + PackageConfig { Exists::Yes, IsValid::No, HasMain::No, "", PackageMode::NONE }); return entry.first->second; } @@ -544,13 +544,11 @@ const PackageConfig& GetPackageConfig(Environment* env, } Local pkg_mode_v; - PackageMode pkg_mode = CJS; - std::string pkg_mode_std; + PackageMode::Mode pkg_mode = PackageMode::CJS; 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; + if (pkg_json->Get(env->context(), env->mode_string()).ToLocal(&pkg_mode_v) && + pkg_mode_v->StrictEquals(env->esm_string())) { + pkg_mode = PackageMode::ESM; } } @@ -560,24 +558,26 @@ const PackageConfig& GetPackageConfig(Environment* env, return entry.first->second; } -PackageMode GetPackageMode(Environment* env, const URL& search) { - URL pjsonPath("package.json", &search); +PackageMode::Mode GetPackageMode(Environment* env, const URL& search) { + URL pjson_path("package.json", &search); while (true) { const PackageConfig& pkg_json = - GetPackageConfig(env, pjsonPath.ToFilePath()); + GetPackageConfig(env, pjson_path.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; + URL last_pjson_path = pjson_path; + pjson_path = URL("../package.json", pjson_path); + // Terminates at root where ../package.json equals ../../package.json + // (can't just check "/package.json" for Windows support). + if (pjson_path.path().length() == last_pjson_path.path().length()) { + return PackageMode::CJS; } } } void SetPackageMode(Environment* env, const URL& search, - PackageMode pkg_mode) { + PackageMode::Mode pkg_mode) { std::string pjsonPathStr = URL("package.json", &search).ToFilePath(); const PackageConfig& pkg_json = GetPackageConfig(env, pjsonPathStr); if (pkg_json.mode != pkg_mode) { @@ -738,17 +738,19 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo& args) { } bool esmPackage = false; - bool set_package_esm_mode = args[2]->ToBoolean().As()->Value(); + bool set_package_esm_mode = args[2]->IsTrue(); if (set_package_esm_mode) { esmPackage = true; - SetPackageMode(env, result.FromJust(), ESM); + SetPackageMode(env, result.FromJust(), PackageMode::ESM); } else { std::string filePath = result.FromJust().ToFilePath(); - // check the package esm mode for ambiguous extensions - if (filePath.substr(filePath.length() - 4, 4) != ".mjs" && - filePath.substr(filePath.length() - 5, 5) != ".json" && - filePath.substr(filePath.length() - 5, 5) != ".node") { - if (GetPackageMode(env, result.FromJust()) == ESM) { + // Check the package esm mode for ambiguous extensions. + if (filePath.length() < 5 || + (filePath.substr(filePath.length() - 4, 4) != ".mjs" && + (filePath.length() < 6 || + (filePath.substr(filePath.length() - 5, 5) != ".json" && + filePath.substr(filePath.length() - 5, 5) != ".node")))) { + if (GetPackageMode(env, result.FromJust()) == PackageMode::ESM) { esmPackage = true; } } @@ -756,17 +758,17 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo& args) { Local resolved = Object::New(env->isolate()); - (void)resolved->DefineOwnProperty( + resolved->DefineOwnProperty( env->context(), env->esm_string(), v8::Boolean::New(env->isolate(), esmPackage), - v8::ReadOnly); + v8::ReadOnly).FromJust(); - (void)resolved->DefineOwnProperty( + resolved->DefineOwnProperty( env->context(), env->url_string(), result.FromJust().ToObject(env), - v8::ReadOnly); + v8::ReadOnly).FromJust(); args.GetReturnValue().Set(resolved); } From 19d92956ed10b61e48f781aa3456c8e5ff2f00fb Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 18 Feb 2018 23:41:58 +0200 Subject: [PATCH 3/5] further PR corrections --- src/module_wrap.cc | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 7b49bad9a7d2..dd7b072baa67 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -545,11 +545,9 @@ const PackageConfig& GetPackageConfig(Environment* env, Local pkg_mode_v; PackageMode::Mode pkg_mode = PackageMode::CJS; - if (pkg_json->Get(env->context(), env->mode_string()).ToLocal(&pkg_mode_v)) { - if (pkg_json->Get(env->context(), env->mode_string()).ToLocal(&pkg_mode_v) && - pkg_mode_v->StrictEquals(env->esm_string())) { - pkg_mode = PackageMode::ESM; - } + if (pkg_json->Get(env->context(), env->mode_string()).ToLocal(&pkg_mode_v) && + pkg_mode_v->StrictEquals(env->esm_string())) { + pkg_mode = PackageMode::ESM; } auto entry = env->package_json_cache.emplace(path, @@ -578,11 +576,11 @@ PackageMode::Mode GetPackageMode(Environment* env, const URL& search) { void SetPackageMode(Environment* env, const URL& search, PackageMode::Mode pkg_mode) { - std::string pjsonPathStr = URL("package.json", &search).ToFilePath(); - const PackageConfig& pkg_json = GetPackageConfig(env, pjsonPathStr); + std::string pjson_path_str = URL("package.json", &search).ToFilePath(); + const PackageConfig& pkg_json = GetPackageConfig(env, pjson_path_str); if (pkg_json.mode != pkg_mode) { - env->package_json_cache.erase(env->package_json_cache.find(pjsonPathStr)); - env->package_json_cache.emplace(pjsonPathStr, + env->package_json_cache.erase(env->package_json_cache.find(pjson_path_str)); + env->package_json_cache.emplace(pjson_path_str, PackageConfig { pkg_json.exists, pkg_json.is_valid, pkg_json.has_main, pkg_json.main, pkg_mode }); } @@ -596,8 +594,8 @@ enum ResolveExtensionsOptions { template Maybe ResolveExtensions(const URL& search) { if (options == TRY_EXACT_NAME) { - std::string filePath = search.ToFilePath(); - Maybe check = CheckFile(filePath); + std::string file_path = search.ToFilePath(); + Maybe check = CheckFile(file_path); if (!check.IsNothing()) { return Just(search); } @@ -737,21 +735,20 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo& args) { return node::THROW_ERR_MISSING_MODULE(env, msg.c_str()); } - bool esmPackage = false; + bool esm_package = false; bool set_package_esm_mode = args[2]->IsTrue(); if (set_package_esm_mode) { - esmPackage = true; + esm_package = true; SetPackageMode(env, result.FromJust(), PackageMode::ESM); } else { - std::string filePath = result.FromJust().ToFilePath(); // Check the package esm mode for ambiguous extensions. - if (filePath.length() < 5 || - (filePath.substr(filePath.length() - 4, 4) != ".mjs" && - (filePath.length() < 6 || - (filePath.substr(filePath.length() - 5, 5) != ".json" && - filePath.substr(filePath.length() - 5, 5) != ".node")))) { + std::string file_path = result.FromJust().ToFilePath(); + std::string ext; + const size_t pos = file_path.rfind('.'); + if (pos != 0 && pos != std::string::npos) ext = file_path.substr(pos); + if (ext != ".mjs" && ext != ".json" && ext != ".node") { if (GetPackageMode(env, result.FromJust()) == PackageMode::ESM) { - esmPackage = true; + esm_package = true; } } } @@ -761,7 +758,7 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo& args) { resolved->DefineOwnProperty( env->context(), env->esm_string(), - v8::Boolean::New(env->isolate(), esmPackage), + v8::Boolean::New(env->isolate(), esm_package), v8::ReadOnly).FromJust(); resolved->DefineOwnProperty( From a92792d6e117dab0fd78468b92e004cbeafdd953 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Fri, 9 Mar 2018 20:09:04 +0200 Subject: [PATCH 4/5] linting fixes --- .eslintrc.js | 8 ++++---- src/module_wrap.cc | 9 ++++++--- test/es-module/test-pjson-cjs-esm-nested.mjs | 2 +- test/es-module/test-pjson-esm-mode-invalid.mjs | 2 +- test/es-module/test-pjson-esm-mode-submodule.mjs | 2 +- test/es-module/test-pjson-esm-mode.mjs | 2 +- 6 files changed, 14 insertions(+), 11 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index e17c10f8bbf5..406f6b10070a 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -34,10 +34,10 @@ module.exports = { overrides: [ { files: [ - "doc/api/esm.md", - "*.mjs", - "test/es-module/esm-dep.js", - "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' }, }, diff --git a/src/module_wrap.cc b/src/module_wrap.cc index dd7b072baa67..4a303c9aafba 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -502,7 +502,8 @@ const PackageConfig& GetPackageConfig(Environment* env, Maybe 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, "", PackageMode::NONE }); + PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "", + PackageMode::NONE }); return entry.first->second; } @@ -520,7 +521,8 @@ 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, "", PackageMode::NONE }); + PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "", + PackageMode::NONE }); return entry.first->second; } @@ -530,7 +532,8 @@ 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, "", PackageMode::NONE }); + PackageConfig { Exists::Yes, IsValid::No, HasMain::No, "", + PackageMode::NONE }); return entry.first->second; } diff --git a/test/es-module/test-pjson-cjs-esm-nested.mjs b/test/es-module/test-pjson-cjs-esm-nested.mjs index 1639abb27db5..ccad1c7410cd 100644 --- a/test/es-module/test-pjson-cjs-esm-nested.mjs +++ b/test/es-module/test-pjson-cjs-esm-nested.mjs @@ -1,5 +1,5 @@ // Flags: --experimental-modules -/* eslint-disable required-modules */ +/* eslint-disable node-core/required-modules */ import m from '../fixtures/es-modules/esm-cjs-nested/module'; import assert from 'assert'; diff --git a/test/es-module/test-pjson-esm-mode-invalid.mjs b/test/es-module/test-pjson-esm-mode-invalid.mjs index 42e9ca3ca49d..3c9e095f2635 100644 --- a/test/es-module/test-pjson-esm-mode-invalid.mjs +++ b/test/es-module/test-pjson-esm-mode-invalid.mjs @@ -1,5 +1,5 @@ // Flags: --experimental-modules -/* eslint-disable required-modules */ +/* eslint-disable node-core/required-modules */ import m from '../fixtures/es-modules/esm-invalid/module'; import assert from 'assert'; diff --git a/test/es-module/test-pjson-esm-mode-submodule.mjs b/test/es-module/test-pjson-esm-mode-submodule.mjs index bc855e2f24c6..770f7743b620 100644 --- a/test/es-module/test-pjson-esm-mode-submodule.mjs +++ b/test/es-module/test-pjson-esm-mode-submodule.mjs @@ -1,5 +1,5 @@ // Flags: --experimental-modules -/* eslint-disable required-modules */ +/* eslint-disable node-core/required-modules */ import m from '../fixtures/es-modules/esm/sub/dir/module'; import assert from 'assert'; diff --git a/test/es-module/test-pjson-esm-mode.mjs b/test/es-module/test-pjson-esm-mode.mjs index 68e899eb672c..fb5c7a8568b0 100644 --- a/test/es-module/test-pjson-esm-mode.mjs +++ b/test/es-module/test-pjson-esm-mode.mjs @@ -1,5 +1,5 @@ // Flags: --experimental-modules -/* eslint-disable required-modules */ +/* eslint-disable node-core/required-modules */ import m from '../fixtures/es-modules/esm/module'; import assert from 'assert'; From 9b1de851badea9868f84f738d990b91f5af0bc99 Mon Sep 17 00:00:00 2001 From: guybedford Date: Tue, 28 Aug 2018 18:28:30 +0200 Subject: [PATCH 5/5] fixup rebase --- src/env.h | 2 +- src/module_wrap.cc | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/env.h b/src/env.h index ae00be451c3c..6a39ddc63a57 100644 --- a/src/env.h +++ b/src/env.h @@ -76,7 +76,7 @@ struct PackageConfig { const IsValid is_valid; const HasMain has_main; const std::string main; - const PackageMode::Mode mode; + const PackageMode mode; }; } // namespace loader diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 4a303c9aafba..faa7825344d2 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -492,6 +492,7 @@ Maybe CheckFile(const std::string& path, using Exists = PackageConfig::Exists; using IsValid = PackageConfig::IsValid; using HasMain = PackageConfig::HasMain; +using PackageMode = PackageConfig::PackageMode; const PackageConfig& GetPackageConfig(Environment* env, const std::string& path) { @@ -547,7 +548,7 @@ const PackageConfig& GetPackageConfig(Environment* env, } Local pkg_mode_v; - PackageMode::Mode pkg_mode = PackageMode::CJS; + PackageMode pkg_mode = PackageMode::CJS; if (pkg_json->Get(env->context(), env->mode_string()).ToLocal(&pkg_mode_v) && pkg_mode_v->StrictEquals(env->esm_string())) { pkg_mode = PackageMode::ESM; @@ -559,7 +560,7 @@ const PackageConfig& GetPackageConfig(Environment* env, return entry.first->second; } -PackageMode::Mode GetPackageMode(Environment* env, const URL& search) { +PackageMode GetPackageMode(Environment* env, const URL& search) { URL pjson_path("package.json", &search); while (true) { const PackageConfig& pkg_json = @@ -578,7 +579,7 @@ PackageMode::Mode GetPackageMode(Environment* env, const URL& search) { } void SetPackageMode(Environment* env, const URL& search, - PackageMode::Mode pkg_mode) { + PackageMode pkg_mode) { std::string pjson_path_str = URL("package.json", &search).ToFilePath(); const PackageConfig& pkg_json = GetPackageConfig(env, pjson_path_str); if (pkg_json.mode != pkg_mode) {