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

module: clarify CJS global-like variables not defined error message #37852

Merged
merged 1 commit into from Apr 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 42 additions & 1 deletion lib/internal/modules/esm/module_job.js
Expand Up @@ -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');
Expand All @@ -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 {
Expand Down Expand Up @@ -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';
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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 say in the ES module ${url}.

Do you want me to add a test with all conditions being true at the same time?

It sounds to me like that is the request here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can just say in the ES module ${url}.

The url is already provided in the first line of the stack trace, is it useful to repeat it here?

Copy link
Contributor

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.


if (StringPrototypeStartsWith(e.message, 'require ')) {
e.message += ', you can use import instead';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps - Use "import" instead or

Copy link
Contributor Author

@aduh95 aduh95 Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is your comment incomplete or? 😅
For reference, the e.message at this point is ReferenceError: require is not defined in ES module scope.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was working to ReferenceError: require is not defined in the ES module ${url} per the previous comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The or was supposed to compose with the .cjs extension suggestion, but we could keep that at the end too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 .js, we need to take into account the case where the module is using a different extension or is a data: URL.

}

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 };
}
}
Expand Down
1 change: 1 addition & 0 deletions lib/internal/modules/esm/resolve.js
Expand Up @@ -893,6 +893,7 @@ module.exports = {
DEFAULT_CONDITIONS,
defaultResolve,
encodedSepRegEx,
getPackageScopeConfig,
getPackageType,
packageExportsResolve,
packageImportsResolve
Expand Down
42 changes: 42 additions & 0 deletions test/es-module/test-esm-undefined-cjs-global-like-variables.js
@@ -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());