Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 718305d

Browse filesBrowse files
mertcanaltinaduh95
mertcanaltin
authored and
aduh95
committed
module: add dynamic file-specific ESM warnings
PR-URL: #56628 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent 0fd3d91 commit 718305d
Copy full SHA for 718305d

File tree

Expand file treeCollapse file tree

5 files changed

+55
-20
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

5 files changed

+55
-20
lines changed
Open diff view settings
Collapse file

‎src/node_contextify.cc‎

Copy file name to clipboardExpand all lines: src/node_contextify.cc
+14-8Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1702,13 +1702,19 @@ static MaybeLocal<Function> CompileFunctionForCJSLoader(
17021702
return scope.Escape(fn);
17031703
}
17041704

1705+
static std::string GetRequireEsmWarning(Local<String> filename) {
1706+
Isolate* isolate = Isolate::GetCurrent();
1707+
Utf8Value filename_utf8(isolate, filename);
1708+
1709+
std::string warning_message =
1710+
"Failed to load the ES module: " + filename_utf8.ToString() +
1711+
". Make sure to set \"type\": \"module\" in the nearest package.json "
1712+
"file "
1713+
"or use the .mjs extension.";
1714+
return warning_message;
1715+
}
1716+
17051717
static bool warned_about_require_esm = false;
1706-
// TODO(joyeecheung): this was copied from the warning previously emitted in the
1707-
// JS land, but it's not very helpful. There should be specific information
1708-
// about which file or which package.json to update.
1709-
const char* require_esm_warning =
1710-
"To load an ES module, set \"type\": \"module\" in the package.json or use "
1711-
"the .mjs extension.";
17121718

17131719
static bool ShouldRetryAsESM(Realm* realm,
17141720
Local<String> message,
@@ -1794,8 +1800,8 @@ static void CompileFunctionForCJSLoader(
17941800
// This needs to call process.emit('warning') in JS which can throw if
17951801
// the user listener throws. In that case, don't try to throw the syntax
17961802
// error.
1797-
should_throw =
1798-
ProcessEmitWarningSync(env, require_esm_warning).IsJust();
1803+
std::string warning_message = GetRequireEsmWarning(filename);
1804+
should_throw = ProcessEmitWarningSync(env, warning_message).IsJust();
17991805
}
18001806
if (should_throw) {
18011807
isolate->ThrowException(cjs_exception);
Collapse file

‎src/node_process_events.cc‎

Copy file name to clipboardExpand all lines: src/node_process_events.cc
+9-1Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ using v8::Just;
1313
using v8::Local;
1414
using v8::Maybe;
1515
using v8::MaybeLocal;
16+
using v8::NewStringType;
1617
using v8::Nothing;
1718
using v8::Object;
1819
using v8::String;
@@ -21,7 +22,14 @@ using v8::Value;
2122
Maybe<bool> ProcessEmitWarningSync(Environment* env, std::string_view message) {
2223
Isolate* isolate = env->isolate();
2324
Local<Context> context = env->context();
24-
Local<String> message_string = OneByteString(isolate, message);
25+
Local<String> message_string;
26+
if (!String::NewFromUtf8(isolate,
27+
message.data(),
28+
NewStringType::kNormal,
29+
static_cast<int>(message.size()))
30+
.ToLocal(&message_string)) {
31+
return Nothing<bool>();
32+
}
2533

2634
Local<Value> argv[] = {message_string};
2735
Local<Function> emit_function = env->process_emit_warning_sync();
Collapse file

‎test/es-module/test-esm-cjs-load-error-note.mjs‎

Copy file name to clipboardExpand all lines: test/es-module/test-esm-cjs-load-error-note.mjs
+1-2Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,10 @@ import assert from 'node:assert';
44
import { execPath } from 'node:process';
55
import { describe, it } from 'node:test';
66

7-
87
// Expect note to be included in the error output
98
// Don't match the following sentence because it can change as features are
109
// added.
11-
const expectedNote = 'Warning: To load an ES module';
10+
const expectedNote = 'Failed to load the ES module';
1211

1312
const mustIncludeMessage = {
1413
getMessage: (stderr) => `${expectedNote} not found in ${stderr}`,
Collapse file

‎test/es-module/test-esm-long-path-win.js‎

Copy file name to clipboardExpand all lines: test/es-module/test-esm-long-path-win.js
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ describe('long path on Windows', () => {
182182
fs.writeFileSync(cjsIndexJSPath, 'import fs from "node:fs/promises";');
183183
const { code, signal, stderr } = await spawnPromisified(execPath, [cjsIndexJSPath]);
184184

185-
assert.ok(stderr.includes('Warning: To load an ES module'));
185+
assert.ok(stderr.includes('Failed to load the ES module'));
186186
assert.strictEqual(code, 1);
187187
assert.strictEqual(signal, null);
188188
});
Collapse file

‎test/es-module/test-typescript-commonjs.mjs‎

Copy file name to clipboardExpand all lines: test/es-module/test-typescript-commonjs.mjs
+30-8Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { skip, spawnPromisified } from '../common/index.mjs';
22
import * as fixtures from '../common/fixtures.mjs';
3-
import { match, strictEqual } from 'node:assert';
3+
import assert, { match, strictEqual } from 'node:assert';
44
import { test } from 'node:test';
55

66
if (!process.config.variables.node_use_amaro) skip('Requires Amaro');
@@ -59,13 +59,35 @@ test('require a .ts file with implicit extension fails', async () => {
5959
});
6060

6161
test('expect failure of an .mts file with CommonJS syntax', async () => {
62-
const result = await spawnPromisified(process.execPath, [
63-
fixtures.path('typescript/cts/test-cts-but-module-syntax.cts'),
64-
]);
65-
66-
strictEqual(result.stdout, '');
67-
match(result.stderr, /To load an ES module, set "type": "module" in the package\.json or use the \.mjs extension\./);
68-
strictEqual(result.code, 1);
62+
const testFilePath = fixtures.path(
63+
'typescript/cts/test-cts-but-module-syntax.cts'
64+
);
65+
const result = await spawnPromisified(process.execPath, [testFilePath]);
66+
67+
assert.strictEqual(result.stdout, '');
68+
69+
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.`;
70+
71+
try {
72+
assert.ok(
73+
result.stderr.includes(expectedWarning),
74+
`Expected stderr to include: ${expectedWarning}`
75+
);
76+
} catch (e) {
77+
if (e?.code === 'ERR_ASSERTION') {
78+
assert.match(
79+
result.stderr,
80+
/Failed to load the ES module:.*test-cts-but-module-syntax\.cts/
81+
);
82+
e.expected = expectedWarning;
83+
e.actual = result.stderr;
84+
e.operator = 'includes';
85+
}
86+
throw e;
87+
}
88+
89+
90+
assert.strictEqual(result.code, 1);
6991
});
7092

7193
test('execute a .cts file importing a .cts file', async () => {

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.