Skip to content

Commit

Permalink
code review notes
Browse files Browse the repository at this point in the history
  • Loading branch information
GeoffreyBooth committed Oct 17, 2023
1 parent 4601209 commit ed954ad
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 13 deletions.
2 changes: 1 addition & 1 deletion lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ translators.set('module', async function moduleStrategy(url, source, isMain) {
*/
function enrichCJSError(err, content, filename) {
if (err != null && ObjectGetPrototypeOf(err) === SyntaxErrorPrototype &&
containsModuleSyntax(content, filename ?? '')) {
containsModuleSyntax(content, filename)) {
// Emit the warning synchronously because we are in the middle of handling
// a SyntaxError that will throw and likely terminate the process before an
// asynchronous warning would be emitted.
Expand Down
29 changes: 17 additions & 12 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1368,17 +1368,19 @@ Local<Object> ContextifyContext::CompileFunctionAndCacheResult(
return result;
}

// These are the error messages thrown due to ESM syntax in a CommonJS module.
// When compiling as CommonJS source code that contains ESM syntax, the
// following error messages are returned:
// - `import` statements: "Cannot use import statement outside a module"
// - `export` statements: "Unexpected token 'export'"
// - `import.meta` references: "Cannot use 'import.meta' outside a module"
// Dynamic `import()` is permitted in CommonJS, so it does not error.
// While top-level `await` is not permitted in CommonJS, it returns the same
// error message as when `await` is used in a sync function, so we don't use it
// as a disambiguation.
constexpr std::array<std::string_view, 3> esm_syntax_error_messages = {
// `import` statements return an error with the message:
"Cannot use import statement outside a module",
// `export` statements return an error with the message:
"Unexpected token 'export'",
// `import.meta` returns an error with the message:
"Cannot use 'import.meta' outside a module"};
// Top-level `await` currently returns the same error message as when `await` is
// used in a sync function, so we don't use it as a disambiguation. Dynamic
// `import()` is permitted in CommonJS, so we don't use it as a disambiguation.
"Cannot use import statement outside a module", // `import` statements
"Unexpected token 'export'", // `export` statements
"Cannot use 'import.meta' outside a module"}; // `import.meta` references

void ContextifyContext::ContainsModuleSyntax(
const FunctionCallbackInfo<Value>& args) {
Expand All @@ -1387,8 +1389,11 @@ void ContextifyContext::ContainsModuleSyntax(
Local<String> code = args[0].As<String>();

// Argument 2: filename
CHECK(args[1]->IsString());
Local<String> filename = args[1].As<String>();
Local<String> filename = String::Empty(args.GetIsolate());
if (!args[1]->IsUndefined()) {
CHECK(args[1]->IsString());
filename = args[1].As<String>();
}

Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();
Expand Down

0 comments on commit ed954ad

Please sign in to comment.