-
Notifications
You must be signed in to change notification settings - Fork 30k
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
module: clarify CJS global-like variables not defined error message #37852
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,18 +4,21 @@ const { | |
ArrayPrototypeJoin, | ||
ArrayPrototypeMap, | ||
ArrayPrototypePush, | ||
ArrayPrototypeSome, | ||
FunctionPrototype, | ||
ObjectSetPrototypeOf, | ||
PromiseAll, | ||
PromiseResolve, | ||
PromisePrototypeCatch, | ||
ReflectApply, | ||
RegExpPrototypeTest, | ||
SafeArrayIterator, | ||
SafeSet, | ||
StringPrototypeIncludes, | ||
StringPrototypeMatch, | ||
StringPrototypeReplace, | ||
StringPrototypeSplit, | ||
StringPrototypeStartsWith, | ||
} = primordials; | ||
|
||
const { ModuleWrap } = internalBinding('module_wrap'); | ||
|
@@ -28,6 +31,19 @@ const noop = FunctionPrototype; | |
|
||
let hasPausedEntry = false; | ||
|
||
const CJSGlobalLike = [ | ||
'require', | ||
'module', | ||
'exports', | ||
'__filename', | ||
'__dirname', | ||
]; | ||
const isCommonJSGlobalLikeNotDefinedError = (errorMessage) => | ||
ArrayPrototypeSome( | ||
CJSGlobalLike, | ||
(globalLike) => errorMessage === `${globalLike} is not defined` | ||
); | ||
|
||
/* A ModuleJob tracks the loading of a single Module, and the ModuleJobs of | ||
* its dependencies, over time. */ | ||
class ModuleJob { | ||
|
@@ -155,7 +171,32 @@ class ModuleJob { | |
await this.instantiate(); | ||
const timeout = -1; | ||
const breakOnSigint = false; | ||
await this.module.evaluate(timeout, breakOnSigint); | ||
try { | ||
await this.module.evaluate(timeout, breakOnSigint); | ||
} catch (e) { | ||
if (e?.name === 'ReferenceError' && | ||
isCommonJSGlobalLikeNotDefinedError(e.message)) { | ||
e.message += ' in ES module scope'; | ||
|
||
if (StringPrototypeStartsWith(e.message, 'require ')) { | ||
e.message += ', you can use import instead'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is your comment incomplete or? 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was working to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see! Note that the second part is printed out only if the module is |
||
} | ||
|
||
const packageConfig = | ||
StringPrototypeStartsWith(this.module.url, 'file://') && | ||
RegExpPrototypeTest(/\.js(\?[^#]*)?(#.*)?$/, this.module.url) && | ||
require('internal/modules/esm/resolve') | ||
.getPackageScopeConfig(this.module.url); | ||
if (packageConfig.type === 'module') { | ||
e.message += | ||
'\nThis file is being treated as an ES module because it has a ' + | ||
`'.js' file extension and '${packageConfig.pjsonPath}' contains ` + | ||
'"type": "module". To treat it as a CommonJS script, rename it ' + | ||
'to use the \'.cjs\' file extension.'; | ||
} | ||
} | ||
throw e; | ||
} | ||
return { module: this.module }; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const fixtures = require('../common/fixtures'); | ||
const assert = require('assert'); | ||
const { pathToFileURL } = require('url'); | ||
|
||
assert.rejects( | ||
import('data:text/javascript,require;'), | ||
/require is not defined in ES module scope, you can use import instead$/ | ||
).then(common.mustCall()); | ||
assert.rejects( | ||
import('data:text/javascript,exports={};'), | ||
/exports is not defined in ES module scope$/ | ||
).then(common.mustCall()); | ||
|
||
assert.rejects( | ||
import('data:text/javascript,require_custom;'), | ||
/^(?!in ES module scope)(?!use import instead).*$/ | ||
).then(common.mustCall()); | ||
|
||
const pkgUrl = pathToFileURL(fixtures.path('/es-modules/package-type-module/')); | ||
assert.rejects( | ||
import(new URL('./cjs.js', pkgUrl)), | ||
/use the '\.cjs' file extension/ | ||
).then(common.mustCall()); | ||
assert.rejects( | ||
import(new URL('./cjs.js#target', pkgUrl)), | ||
/use the '\.cjs' file extension/ | ||
).then(common.mustCall()); | ||
assert.rejects( | ||
import(new URL('./cjs.js?foo=bar', pkgUrl)), | ||
/use the '\.cjs' file extension/ | ||
).then(common.mustCall()); | ||
assert.rejects( | ||
import(new URL('./cjs.js?foo=bar#target', pkgUrl)), | ||
/use the '\.cjs' file extension/ | ||
).then(common.mustCall()); | ||
|
||
assert.rejects( | ||
import('data:text/javascript,require;//.js'), | ||
/^(?!use the '\.cjs' file extension).*$/ | ||
).then(common.mustCall()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more than one of these if statements can be true at the same time. we should ensure every combination is a valid sentence or sentences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I think it is already, isn't it? Do you want me to add a test with all conditions being true at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of
in ES module scope
perhaps we can just sayin the ES module ${url}
.It sounds to me like that is the request here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
url
is already provided in the first line of the stack trace, is it useful to repeat it here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good poin - if we definitely have an error frame for all of these then we should work to that.