From 777a249faddd32ff5962f61d96a00afc26f62657 Mon Sep 17 00:00:00 2001 From: Mert Can Altin Date: Thu, 16 Jan 2025 18:17:29 +0300 Subject: [PATCH 1/8] module: add dynamic file-specific ESM warnings --- src/node_contextify.cc | 21 ++++++++++++------- .../test-esm-cjs-load-error-note.mjs | 3 +-- test/es-module/test-typescript-commonjs.mjs | 10 +++++---- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 77d35675827c67..8e779bbbfd276c 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -1688,13 +1688,19 @@ static MaybeLocal CompileFunctionForCJSLoader( return scope.Escape(fn); } +static std::string GetRequireEsmWarning(Local filename) { + Isolate* isolate = Isolate::GetCurrent(); + Utf8Value filename_utf8(isolate, filename); + + std::string warning_message = + "Failed to load the ES module: " + std::string(*filename_utf8) + + ". Make sure to set \"type\": \"module\" in the nearest package.json " + "file " + "or use the .mjs extension."; + return warning_message; +} + static bool warned_about_require_esm = false; -// TODO(joyeecheung): this was copied from the warning previously emitted in the -// JS land, but it's not very helpful. There should be specific information -// about which file or which package.json to update. -const char* require_esm_warning = - "To load an ES module, set \"type\": \"module\" in the package.json or use " - "the .mjs extension."; static bool ShouldRetryAsESM(Realm* realm, Local message, @@ -1780,8 +1786,9 @@ static void CompileFunctionForCJSLoader( // This needs to call process.emit('warning') in JS which can throw if // the user listener throws. In that case, don't try to throw the syntax // error. + std::string warning_message = GetRequireEsmWarning(filename); should_throw = - ProcessEmitWarningSync(env, require_esm_warning).IsJust(); + ProcessEmitWarningSync(env, warning_message.c_str()).IsJust(); } if (should_throw) { isolate->ThrowException(cjs_exception); diff --git a/test/es-module/test-esm-cjs-load-error-note.mjs b/test/es-module/test-esm-cjs-load-error-note.mjs index 2875f4844de359..54000b53fea9b4 100644 --- a/test/es-module/test-esm-cjs-load-error-note.mjs +++ b/test/es-module/test-esm-cjs-load-error-note.mjs @@ -4,11 +4,10 @@ import assert from 'node:assert'; import { execPath } from 'node:process'; import { describe, it } from 'node:test'; - // Expect note to be included in the error output // Don't match the following sentence because it can change as features are // added. -const expectedNote = 'Warning: To load an ES module'; +const expectedNote = 'Failed to load the ES module'; const mustIncludeMessage = { getMessage: (stderr) => `${expectedNote} not found in ${stderr}`, diff --git a/test/es-module/test-typescript-commonjs.mjs b/test/es-module/test-typescript-commonjs.mjs index 5b15860ab32796..a77d49cb3cbfac 100644 --- a/test/es-module/test-typescript-commonjs.mjs +++ b/test/es-module/test-typescript-commonjs.mjs @@ -59,12 +59,14 @@ test('require a .ts file with implicit extension fails', async () => { }); test('expect failure of an .mts file with CommonJS syntax', async () => { - const result = await spawnPromisified(process.execPath, [ - fixtures.path('typescript/cts/test-cts-but-module-syntax.cts'), - ]); + const testFilePath = fixtures.path('typescript/cts/test-cts-but-module-syntax.cts'); + const result = await spawnPromisified(process.execPath, [testFilePath]); strictEqual(result.stdout, ''); - match(result.stderr, /To load an ES module, set "type": "module" in the package\.json or use the \.mjs extension\./); + + const expectedWarning = `Failed to load the ES module: ${testFilePath}. Make sure to set "type": "module" in the nearest package.json file or use the .mjs extension.`; + match(result.stderr, new RegExp(expectedWarning)); + strictEqual(result.code, 1); }); From 5541de0b0907b72553a201c003d1346a1c585a54 Mon Sep 17 00:00:00 2001 From: Mert Can Altin Date: Sun, 19 Jan 2025 17:11:02 +0300 Subject: [PATCH 2/8] Update src/node_contextify.cc Co-authored-by: Anna Henningsen --- src/node_contextify.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 8e779bbbfd276c..ada540c4d3edd0 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -1693,7 +1693,7 @@ static std::string GetRequireEsmWarning(Local filename) { Utf8Value filename_utf8(isolate, filename); std::string warning_message = - "Failed to load the ES module: " + std::string(*filename_utf8) + + "Failed to load the ES module: " + filename_utf8.ToString() + ". Make sure to set \"type\": \"module\" in the nearest package.json " "file " "or use the .mjs extension."; From 87b5e2e744dad9a92de90f306259878e9482f104 Mon Sep 17 00:00:00 2001 From: Mert Can Altin Date: Fri, 24 Jan 2025 08:15:08 +0300 Subject: [PATCH 3/8] Update test-typescript-commonjs.mjs Co-authored-by: Antoine du Hamel --- test/es-module/test-typescript-commonjs.mjs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/es-module/test-typescript-commonjs.mjs b/test/es-module/test-typescript-commonjs.mjs index a77d49cb3cbfac..649d02a04b0041 100644 --- a/test/es-module/test-typescript-commonjs.mjs +++ b/test/es-module/test-typescript-commonjs.mjs @@ -65,7 +65,16 @@ test('expect failure of an .mts file with CommonJS syntax', async () => { strictEqual(result.stdout, ''); const expectedWarning = `Failed to load the ES module: ${testFilePath}. Make sure to set "type": "module" in the nearest package.json file or use the .mjs extension.`; - match(result.stderr, new RegExp(expectedWarning)); + try { + assert.ok(result.stderr.includes(expectedWarning), 'stderr does not contain the expected warning'); + } catch (e) { + if (e?.code === 'ERR_ASSERTION') { + e.expected = expectedWarning; + e.actual = result.stderr; + e.operator = 'includes'; + } + throw e; + } strictEqual(result.code, 1); }); From 23b8955df2beeff84226e0edda90a172cf356ddd Mon Sep 17 00:00:00 2001 From: Mert Can Altin Date: Fri, 24 Jan 2025 08:26:11 +0300 Subject: [PATCH 4/8] lint --- test/es-module/test-typescript-commonjs.mjs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/es-module/test-typescript-commonjs.mjs b/test/es-module/test-typescript-commonjs.mjs index 649d02a04b0041..aaf13b05b1a989 100644 --- a/test/es-module/test-typescript-commonjs.mjs +++ b/test/es-module/test-typescript-commonjs.mjs @@ -1,6 +1,6 @@ import { skip, spawnPromisified } from '../common/index.mjs'; import * as fixtures from '../common/fixtures.mjs'; -import { match, strictEqual } from 'node:assert'; +import { match, strictEqual, assert } from 'node:assert'; import { test } from 'node:test'; if (!process.config.variables.node_use_amaro) skip('Requires Amaro'); @@ -65,6 +65,7 @@ test('expect failure of an .mts file with CommonJS syntax', async () => { strictEqual(result.stdout, ''); const expectedWarning = `Failed to load the ES module: ${testFilePath}. Make sure to set "type": "module" in the nearest package.json file or use the .mjs extension.`; + try { assert.ok(result.stderr.includes(expectedWarning), 'stderr does not contain the expected warning'); } catch (e) { From 05c573f154e2e520d651e547f582cbc20ce503d0 Mon Sep 17 00:00:00 2001 From: Mert Can Altin Date: Fri, 24 Jan 2025 14:48:48 +0300 Subject: [PATCH 5/8] fix: resolved import issue with assert module --- test/es-module/test-typescript-commonjs.mjs | 76 ++++++++++----------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/test/es-module/test-typescript-commonjs.mjs b/test/es-module/test-typescript-commonjs.mjs index aaf13b05b1a989..dc0215c1066893 100644 --- a/test/es-module/test-typescript-commonjs.mjs +++ b/test/es-module/test-typescript-commonjs.mjs @@ -1,6 +1,6 @@ import { skip, spawnPromisified } from '../common/index.mjs'; import * as fixtures from '../common/fixtures.mjs'; -import { match, strictEqual, assert } from 'node:assert'; +import * as assert from 'node:assert'; import { test } from 'node:test'; if (!process.config.variables.node_use_amaro) skip('Requires Amaro'); @@ -14,9 +14,9 @@ test('require a .ts file with explicit extension succeeds', async () => { cwd: fixtures.path('typescript/ts'), }); - strictEqual(result.stderr, ''); - strictEqual(result.stdout, 'Hello, TypeScript!\n'); - strictEqual(result.code, 0); + assert.strictEqual(result.stderr, ''); + assert.strictEqual(result.stdout, 'Hello, TypeScript!\n'); + assert.strictEqual(result.code, 0); }); test('eval require a .ts file with implicit extension fails', async () => { @@ -28,9 +28,9 @@ test('eval require a .ts file with implicit extension fails', async () => { cwd: fixtures.path('typescript/ts'), }); - strictEqual(result.stdout, ''); - match(result.stderr, /Error: Cannot find module/); - strictEqual(result.code, 1); + assert.strictEqual(result.stdout, ''); + assert.match(result.stderr, /Error: Cannot find module/); + assert.strictEqual(result.code, 1); }); test('eval require a .cts file with implicit extension fails', async () => { @@ -42,9 +42,9 @@ test('eval require a .cts file with implicit extension fails', async () => { cwd: fixtures.path('typescript/ts'), }); - strictEqual(result.stdout, ''); - match(result.stderr, /Error: Cannot find module/); - strictEqual(result.code, 1); + assert.strictEqual(result.stdout, ''); + assert.match(result.stderr, /Error: Cannot find module/); + assert.strictEqual(result.code, 1); }); test('require a .ts file with implicit extension fails', async () => { @@ -53,16 +53,16 @@ test('require a .ts file with implicit extension fails', async () => { fixtures.path('typescript/cts/test-extensionless-require.ts'), ]); - strictEqual(result.stdout, ''); - match(result.stderr, /Error: Cannot find module/); - strictEqual(result.code, 1); + assert.strictEqual(result.stdout, ''); + assert.match(result.stderr, /Error: Cannot find module/); + assert.strictEqual(result.code, 1); }); test('expect failure of an .mts file with CommonJS syntax', async () => { const testFilePath = fixtures.path('typescript/cts/test-cts-but-module-syntax.cts'); const result = await spawnPromisified(process.execPath, [testFilePath]); - strictEqual(result.stdout, ''); + assert.strictEqual(result.stdout, ''); const expectedWarning = `Failed to load the ES module: ${testFilePath}. Make sure to set "type": "module" in the nearest package.json file or use the .mjs extension.`; @@ -77,7 +77,7 @@ test('expect failure of an .mts file with CommonJS syntax', async () => { throw e; } - strictEqual(result.code, 1); + assert.strictEqual(result.code, 1); }); test('execute a .cts file importing a .cts file', async () => { @@ -86,9 +86,9 @@ test('execute a .cts file importing a .cts file', async () => { fixtures.path('typescript/cts/test-require-commonjs.cts'), ]); - strictEqual(result.stderr, ''); - match(result.stdout, /Hello, TypeScript!/); - strictEqual(result.code, 0); + assert.strictEqual(result.stderr, ''); + assert.match(result.stdout, /Hello, TypeScript!/); + assert.strictEqual(result.code, 0); }); test('execute a .cts file importing a .ts file export', async () => { @@ -97,9 +97,9 @@ test('execute a .cts file importing a .ts file export', async () => { fixtures.path('typescript/cts/test-require-ts-file.cts'), ]); - strictEqual(result.stderr, ''); - match(result.stdout, /Hello, TypeScript!/); - strictEqual(result.code, 0); + assert.strictEqual(result.stderr, ''); + assert.match(result.stdout, /Hello, TypeScript!/); + assert.strictEqual(result.code, 0); }); test('execute a .cts file importing a .mts file export', async () => { @@ -108,9 +108,9 @@ test('execute a .cts file importing a .mts file export', async () => { fixtures.path('typescript/cts/test-require-mts-module.cts'), ]); - strictEqual(result.stdout, ''); - match(result.stderr, /Error \[ERR_REQUIRE_ESM\]: require\(\) of ES Module/); - strictEqual(result.code, 1); + assert.strictEqual(result.stdout, ''); + assert.match(result.stderr, /Error \[ERR_REQUIRE_ESM\]: require\(\) of ES Module/); + assert.strictEqual(result.code, 1); }); test('execute a .cts file importing a .mts file export', async () => { @@ -119,8 +119,8 @@ test('execute a .cts file importing a .mts file export', async () => { fixtures.path('typescript/cts/test-require-mts-module.cts'), ]); - match(result.stdout, /Hello, TypeScript!/); - strictEqual(result.code, 0); + assert.match(result.stdout, /Hello, TypeScript!/); + assert.strictEqual(result.code, 0); }); test('expect failure of a .cts file in node_modules', async () => { @@ -128,9 +128,9 @@ test('expect failure of a .cts file in node_modules', async () => { fixtures.path('typescript/cts/test-cts-node_modules.cts'), ]); - strictEqual(result.stdout, ''); - match(result.stderr, /ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING/); - strictEqual(result.code, 1); + assert.strictEqual(result.stdout, ''); + assert.match(result.stderr, /ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING/); + assert.strictEqual(result.code, 1); }); test('expect failure of a .ts file in node_modules', async () => { @@ -138,9 +138,9 @@ test('expect failure of a .ts file in node_modules', async () => { fixtures.path('typescript/cts/test-ts-node_modules.cts'), ]); - strictEqual(result.stdout, ''); - match(result.stderr, /ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING/); - strictEqual(result.code, 1); + assert.strictEqual(result.stdout, ''); + assert.match(result.stderr, /ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING/); + assert.strictEqual(result.code, 1); }); test('expect failure of a .cts requiring esm without default type module', async () => { @@ -149,9 +149,9 @@ test('expect failure of a .cts requiring esm without default type module', async fixtures.path('typescript/cts/test-mts-node_modules.cts'), ]); - strictEqual(result.stdout, ''); - match(result.stderr, /ERR_REQUIRE_ESM/); - strictEqual(result.code, 1); + assert.strictEqual(result.stdout, ''); + assert.match(result.stderr, /ERR_REQUIRE_ESM/); + assert.strictEqual(result.code, 1); }); test('expect failure of a .cts file requiring esm in node_modules', async () => { @@ -160,7 +160,7 @@ test('expect failure of a .cts file requiring esm in node_modules', async () => fixtures.path('typescript/cts/test-mts-node_modules.cts'), ]); - strictEqual(result.stdout, ''); - match(result.stderr, /ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING/); - strictEqual(result.code, 1); + assert.strictEqual(result.stdout, ''); + assert.match(result.stderr, /ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING/); + assert.strictEqual(result.code, 1); }); From 17eb1d494cfbdff73f01745bb5886a13fc9d3c0c Mon Sep 17 00:00:00 2001 From: Mert Can Altin Date: Wed, 12 Feb 2025 19:20:43 +0300 Subject: [PATCH 6/8] conversion error in UTF-8 string creation --- src/node_process_events.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/node_process_events.cc b/src/node_process_events.cc index 8aac953b3e0db5..c77097c75b1e7f 100644 --- a/src/node_process_events.cc +++ b/src/node_process_events.cc @@ -21,7 +21,12 @@ using v8::Value; Maybe ProcessEmitWarningSync(Environment* env, std::string_view message) { Isolate* isolate = env->isolate(); Local context = env->context(); - Local message_string = OneByteString(isolate, message); + v8::Local message_string = + v8::String::NewFromUtf8(isolate, + message.data(), + v8::NewStringType::kNormal, + static_cast(message.size())) + .ToLocalChecked(); Local argv[] = {message_string}; Local emit_function = env->process_emit_warning_sync(); From 7f2ff3f31ec01f3bf00e4cff0d88d7f801386c04 Mon Sep 17 00:00:00 2001 From: Mert Can Altin Date: Fri, 14 Feb 2025 16:45:03 +0300 Subject: [PATCH 7/8] Update node_contextify.cc Co-authored-by: Anna Henningsen --- src/node_contextify.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index ada540c4d3edd0..54aeaaac620de9 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -1788,7 +1788,7 @@ static void CompileFunctionForCJSLoader( // error. std::string warning_message = GetRequireEsmWarning(filename); should_throw = - ProcessEmitWarningSync(env, warning_message.c_str()).IsJust(); + ProcessEmitWarningSync(env, warning_message).IsJust(); } if (should_throw) { isolate->ThrowException(cjs_exception); From da45468b6fc942aed3907b929c412c87a4fe6f0d Mon Sep 17 00:00:00 2001 From: Mert Can Altin Date: Fri, 14 Feb 2025 17:28:26 +0300 Subject: [PATCH 8/8] lint --- src/node_contextify.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 54aeaaac620de9..ba2470f4dced8e 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -1787,8 +1787,7 @@ static void CompileFunctionForCJSLoader( // the user listener throws. In that case, don't try to throw the syntax // error. std::string warning_message = GetRequireEsmWarning(filename); - should_throw = - ProcessEmitWarningSync(env, warning_message).IsJust(); + should_throw = ProcessEmitWarningSync(env, warning_message).IsJust(); } if (should_throw) { isolate->ThrowException(cjs_exception);