Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esm: Implement esm mode flag #18392

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
3 changes: 2 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ module.exports = {
files: [
'doc/api/esm.md',
'*.mjs',
'test/es-module/test-esm-example-loader.js',
'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
97 changes: 88 additions & 9 deletions src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ Maybe<uv_file> 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) {
Expand All @@ -502,7 +503,8 @@ 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, "",
PackageMode::NONE });
return entry.first->second;
}

Expand All @@ -520,7 +522,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, "" });
PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "",
PackageMode::NONE });
return entry.first->second;
}

Expand All @@ -530,7 +533,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, "" });
PackageConfig { Exists::Yes, IsValid::No, HasMain::No, "",
PackageMode::NONE });
return entry.first->second;
}

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

Local<Value> pkg_mode_v;
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;
}
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 pjson_path("package.json", &search);
while (true) {
const PackageConfig& pkg_json =
GetPackageConfig(env, pjson_path.ToFilePath());
if (pkg_json.exists == Exists::Yes) {
return pkg_json.mode;
}
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;
}
}
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 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(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 });
}
}

enum ResolveExtensionsOptions {
TRY_EXACT_NAME,
ONLY_VIA_EXTENSIONS
Expand All @@ -556,8 +598,8 @@ enum ResolveExtensionsOptions {
template <ResolveExtensionsOptions options>
Maybe<URL> ResolveExtensions(const URL& search) {
if (options == TRY_EXACT_NAME) {
std::string filePath = search.ToFilePath();
Maybe<uv_file> check = CheckFile(filePath);
std::string file_path = search.ToFilePath();
Maybe<uv_file> check = CheckFile(file_path);
if (!check.IsNothing()) {
return Just(search);
}
Expand Down Expand Up @@ -670,8 +712,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 +728,50 @@ 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 esm_package = false;
bool set_package_esm_mode = args[2]->IsTrue();
if (set_package_esm_mode) {
esm_package = true;
SetPackageMode(env, result.FromJust(), PackageMode::ESM);
} else {
// Check the package esm mode for ambiguous extensions.
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) {
esm_package = true;
}
}
}

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

resolved->DefineOwnProperty(
env->context(),
env->esm_string(),
v8::Boolean::New(env->isolate(), esm_package),
v8::ReadOnly).FromJust();

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

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 node-core/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 node-core/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 node-core/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 node-core/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.