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: proxy require('esm') to import('esm') #39064

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 0 additions & 20 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1396,26 +1396,6 @@ E('ERR_PACKAGE_PATH_NOT_EXPORTED', (pkgPath, subpath, base = undefined) => {
E('ERR_PERFORMANCE_INVALID_TIMESTAMP',
'%d is not a valid timestamp', TypeError);
E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError);
E('ERR_REQUIRE_ESM',
(filename, parentPath = null, packageJsonPath = null) => {
let msg = `Must use import to load ES Module: ${filename}`;
if (parentPath && packageJsonPath) {
const path = require('path');
const basename = path.basename(filename) === path.basename(parentPath) ?
filename : path.basename(filename);
msg +=
'\nrequire() of ES modules is not supported.\nrequire() of ' +
`${filename} from ${parentPath} ` +
'is an ES module file as it is a .js file whose nearest parent ' +
'package.json contains "type": "module" which defines all .js ' +
'files in that package scope as ES modules.\nInstead rename ' +
`${basename} to end in .cjs, change the requiring code to use ` +
'import(), or remove "type": "module" from ' +
`${packageJsonPath}.\n`;
return msg;
}
return msg;
}, Error);
E('ERR_SCRIPT_EXECUTION_INTERRUPTED',
'Script execution was interrupted by `SIGINT`', Error);
E('ERR_SERVER_ALREADY_LISTEN',
Expand Down
26 changes: 19 additions & 7 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ let hasLoadedAnyUserCJSModule = false;
const {
ERR_INVALID_ARG_VALUE,
ERR_INVALID_MODULE_SPECIFIER,
ERR_REQUIRE_ESM,
ERR_UNKNOWN_BUILTIN_MODULE,
} = require('internal/errors').codes;
const { validateString } = require('internal/validators');
Expand Down Expand Up @@ -983,8 +982,17 @@ Module.prototype.load = function(filename) {

const extension = findLongestRegisteredExtension(filename);
// allow .mjs to be overridden
if (StringPrototypeEndsWith(filename, '.mjs') && !Module._extensions['.mjs'])
throw new ERR_REQUIRE_ESM(filename);
if (StringPrototypeEndsWith(filename, '.mjs') &&
!Module._extensions['.mjs']) {
process.emitWarning('Attempting to require and ESModule.' +
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
process.emitWarning('Attempting to require and ESModule.' +
process.emitWarning('Attempting to require and ESModule. ' +

Tests

'Replace `require` with `import`',
'Warning', 'WARN_REQUIRE_ESM');
const ESMLoader = asyncESM.ESMLoader;
const exports = ESMLoader.import(filename);
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be converted to a URL for windows paths.

Copy link

@weswigham weswigham Jun 21, 2021

Choose a reason for hiding this comment

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

It also makes sense to expose the inners of getModuleJob directly and use that rather than import (namely all the bits after the specifier is resolved, like in the other PR) - the cjs loader has already resolved the specifier at this point so there's no reason to have the esm loader double resolve it. (That's just more unneeded disk hits, and potential format confusion bugs if the two loaders disagree on the format)

this.exports = exports;
this.loaded = true;
return;
}

Module._extensions[extension](this, filename);
this.loaded = true;
Expand Down Expand Up @@ -1120,10 +1128,14 @@ Module._extensions['.js'] = function(module, filename) {
const pkg = readPackageScope(filename);
// Function require shouldn't be used in ES modules.
if (pkg?.data?.type === 'module') {
const parent = moduleParentCache.get(module);
const parentPath = parent?.filename;
const packageJsonPath = path.resolve(pkg.path, 'package.json');
throw new ERR_REQUIRE_ESM(filename, parentPath, packageJsonPath);
process.emitWarning('Attempting to require and ESModule.' +
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
process.emitWarning('Attempting to require and ESModule.' +
process.emitWarning('Attempting to require and ESModule. ' +

Tests

'Replace `require` with `import`',
'Warning', 'WARN_REQUIRE_ESM');
const ESMLoader = asyncESM.ESMLoader;
const exports = ESMLoader.import(filename);
module.exports = exports;
module.loaded = true;
return;
}
}
// If already analyzed the source, then it will be cached.
Expand Down
41 changes: 0 additions & 41 deletions test/es-module/test-cjs-esm-warn.js

This file was deleted.

10 changes: 0 additions & 10 deletions test/es-module/test-esm-type-flag-errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,6 @@ expect('', packageWithoutTypeMain, 'package-without-type');
expect('--input-type=module', packageTypeModuleMain,
'ERR_INPUT_TYPE_NOT_ALLOWED', true);

try {
require('../fixtures/es-modules/package-type-module/index.js');
assert.fail('Expected CJS to fail loading from type: module package.');
} catch (e) {
assert.strictEqual(e.name, 'Error');
assert.strictEqual(e.code, 'ERR_REQUIRE_ESM');
assert(e.toString().match(/Must use import to load ES Module/g));
assert(e.message.match(/Must use import to load ES Module/g));
}

function expect(opt = '', inputFile, want, wantsError = false) {
const argv = [inputFile];
const opts = {
Expand Down
27 changes: 19 additions & 8 deletions test/parallel/test-require-mjs.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,22 @@
'use strict';
require('../common');
const { expectWarning } = require('../common');
const assert = require('assert');

assert.throws(
() => require('../fixtures/es-modules/test-esm-ok.mjs'),
{
message: /Must use import to load ES Module/,
code: 'ERR_REQUIRE_ESM'
}
);
expectWarning('Warning', [
[
'Attempting to require and ESModule. Replace `require` with `import`',
'WARN_REQUIRE_ESM',
],
[
'Attempting to require and ESModule. Replace `require` with `import`',
'WARN_REQUIRE_ESM',
],
]);

require('../fixtures/es-modules/test-esm-ok.mjs').then((module) => {
assert.ok(module.default);
});

require('../fixtures/es-modules/package-type-module').then((module) => {
assert.strictEqual(module.default, 'package-type-module');
});