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

[ESM] always load the ESM loader #279

Merged
merged 3 commits into from Mar 26, 2022
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
6 changes: 0 additions & 6 deletions lib/get-source-loader.mjs

This file was deleted.

8 changes: 1 addition & 7 deletions lib/hook.js
@@ -1,6 +1,6 @@
const vm = require('vm');

module.exports = (patchVM, wrapper, callback) => {
module.exports = (patchVM, callback) => {
// Hook into Node's `require(...)`
updateHooks();

Expand Down Expand Up @@ -48,12 +48,6 @@ module.exports = (patchVM, wrapper, callback) => {
*/
function createHook(handler) {
return function nodeDevHook(module, filename) {
if (module.parent === wrapper) {
// If the main module is required conceal the wrapper
module.id = '.';
module.parent = null;
process.mainModule = module;
}
if (!module.loaded) callback(module.filename);

// Invoke the original handler
Expand Down
38 changes: 16 additions & 22 deletions lib/index.js
@@ -1,8 +1,7 @@
const { fork } = require('child_process');
const filewatcher = require('filewatcher');
const { extname } = require('path');
const semver = require('semver');
const getPackageType = require('get-package-type');
const { pathToFileURL } = require('url');

const { clearFactory } = require('./clear');
const { configureDeps, configureIgnore } = require('./ignore');
Expand Down Expand Up @@ -55,8 +54,6 @@ module.exports = function (
const isIgnored = configureIgnore(ignore);
const isTooDeep = configureDeps(deps);

const wrapper = resolveMain(localPath('wrap.js'));

// Run ./dedupe.js as preload script
if (dedupe) process.env.NODE_DEV_PRELOAD = localPath('dedupe');

Expand Down Expand Up @@ -96,27 +93,24 @@ module.exports = function (

const args = nodeArgs.slice();

if (extname(script) === '.mjs' || getPackageType.sync(script) === 'module') {
if (semver.satisfies(process.version, '>=10 <12.11.1')) {
const resolveLoader = resolveMain(localPath('resolve-loader.mjs'));
args.push('--experimental-modules', `--loader=${resolveLoader}`);
} else if (semver.satisfies(process.version, '>=12.11.1')) {
if (semver.satisfies(process.version, '<12.17.0')) {
args.push('--experimental-modules');
}
const loaderPath = semver.satisfies(process.version, '>=16.12.0')
? 'load-loader.mjs'
: 'get-source-loader.mjs';
const experimentalLoader = resolveMain(localPath(loaderPath));
args.push(`--experimental-loader=${experimentalLoader}`);
}
args.push(`--require=${resolveMain(localPath('wrap'))}`);
const loaderPath = semver.satisfies(process.version, '<16.12.0')
? 'loader-legacy.mjs'
: 'loader.mjs';
const loader = pathToFileURL(resolveMain(localPath(loaderPath)));
if (semver.satisfies(process.version, '<12.17.0')) {
args.push('--experimental-modules');
}
if (semver.satisfies(process.version, '<12.11.1')) {
args.push(`--loader=${loader.href}`);
} else {
args.push(`--experimental-loader=${loader.href}`);
}

const cmd = args.concat(wrapper, script, scriptArgs);

child = fork(cmd[0], cmd.slice(1), {
child = fork(script, scriptArgs, {
cwd: process.cwd(),
env: process.env
env: process.env,
execArgv: args
});

if (respawn) {
Expand Down
6 changes: 0 additions & 6 deletions lib/load-loader.mjs

This file was deleted.

20 changes: 20 additions & 0 deletions lib/loader-legacy.mjs
@@ -0,0 +1,20 @@
import { createRequire } from 'module';
import { fileURLToPath } from 'url';
import { send } from './ipc.mjs';

const require = createRequire(import.meta.url);

export async function getFormat(url, context, defaultGetFormat) {
const getPackageType = require('get-package-type');
const filePath = fileURLToPath(url);
Copy link
Contributor

@lehni lehni Mar 27, 2022

Choose a reason for hiding this comment

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

@bjornstar @kherock this crashes on any import of built in modules now, of which the URL is in the format node:<identifier>, e.g. node:path:

TypeError [ERR_INVALID_URL_SCHEME]: The URL must be of scheme file

Copy link
Contributor

Choose a reason for hiding this comment

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

I created an issue for this with a little test-case: #281


send({ required: filePath });
try {
return await defaultGetFormat(url, context, defaultGetFormat);
} catch (err) {
if (err.code === 'ERR_UNKNOWN_FILE_EXTENSION') {
return { format: await getPackageType(filePath) };
}
throw err;
}
}
20 changes: 20 additions & 0 deletions lib/loader.mjs
@@ -0,0 +1,20 @@
import { createRequire } from 'module';
import { fileURLToPath } from 'url';
import { send } from './ipc.mjs';

const require = createRequire(import.meta.url);

export async function load(url, context, defaultLoad) {
const getPackageType = require('get-package-type');
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this is only used in the unknown file extension case. Let's get it out of the critical path.

const filePath = fileURLToPath(url);

send({ required: filePath });
try {
return await defaultLoad(url, context, defaultLoad);
} catch (err) {
if (err.code === 'ERR_UNKNOWN_FILE_EXTENSION') {
return { format: await getPackageType(filePath), source: null };
}
throw err;
}
Comment on lines +12 to +19
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
try {
return await defaultLoad(url, context, defaultLoad);
} catch (err) {
if (err.code === 'ERR_UNKNOWN_FILE_EXTENSION') {
return { format: await getPackageType(filePath), source: null };
}
throw err;
}
return defaultLoad(url, context, defaultLoad).catch(error => {
if (error.code !== 'ERR_UNKNOWN_FILE_EXTENSION') throw error;
return require('get-package-type')(filePath).then(format => ({ format, source: null });
});

}
13 changes: 0 additions & 13 deletions lib/resolve-loader.mjs

This file was deleted.

13 changes: 13 additions & 0 deletions lib/suppress-experimental.js
@@ -0,0 +1,13 @@
const { emitWarning } = process;

process.emitWarning = (warning, ...args) => {
if (args[0] === 'ExperimentalWarning') {
return;
}

if (args[0] && typeof args[0] === 'object' && args[0].type === 'ExperimentalWarning') {
return;
}

return emitWarning(warning, ...args);
};
12 changes: 3 additions & 9 deletions lib/wrap.js
@@ -1,22 +1,19 @@
const { dirname, extname } = require('path');
const childProcess = require('child_process');
const { sync: resolve } = require('resolve');
const getPackageType = require('get-package-type');

const { getConfig } = require('./cfg');
const hook = require('./hook');
const { relay, send } = require('./ipc');
const resolveMain = require('./resolve-main');

// Remove wrap.js from the argv array
process.argv.splice(1, 1);

const script = process.argv[1];
const { extensions, fork, vm } = getConfig(script);

if (process.env.NODE_DEV_PRELOAD) {
require(process.env.NODE_DEV_PRELOAD);
}
require('./suppress-experimental');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should be hiding warnings by default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although I can see now that it gets quite annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also trying to hold off on implementing this but I figure that this way is probably for the best since it was affecting the test output a lot!


// We want to exit on SIGTERM, but defer to existing SIGTERM handlers.
process.once('SIGTERM', () => process.listenerCount('SIGTERM') || process.exit(0));
Expand All @@ -26,7 +23,7 @@ if (fork) {
// too. We also need to relay messages about required files to the parent.
const originalFork = childProcess.fork;
childProcess.fork = (modulePath, args, options) => {
const child = originalFork(__filename, [modulePath].concat(args), options);
const child = originalFork(modulePath, args, options);
relay(child);
return child;
};
Expand All @@ -48,7 +45,7 @@ process.on('uncaughtException', err => {
});

// Hook into require() and notify the parent process about required files
hook(vm, module, required => send({ required }));
hook(vm, required => send({ required }));

// Check if a module is registered for this extension
const main = resolveMain(script);
Expand All @@ -66,6 +63,3 @@ if (typeof mod === 'object' && mod.name) {
} else if (typeof mod === 'string') {
require(resolve(mod, { basedir }));
}

// Execute the wrapped script
ext === 'mjs' || getPackageType.sync(main) === 'module' ? import(main) : require(main);
2 changes: 1 addition & 1 deletion test/spawn/argv.js
Expand Up @@ -6,7 +6,7 @@ tap.test('should not show up in argv', t => {
spawn('argv.js foo', out => {
const argv = JSON.parse(out.replace(/'/g, '"'));
t.match(argv[0], /.*?node(js|\.exe)?$/);
t.equal(argv[1], 'argv.js');
t.match(argv[1], /.*[\\/]argv\.js$/);
t.equal(argv[2], 'foo');
return { exit: t.end.bind(t) };
});
Expand Down
7 changes: 0 additions & 7 deletions test/spawn/esmodule.js
Expand Up @@ -6,7 +6,6 @@ const { spawn, touchFile } = require('../utils');
tap.test('Supports ECMAScript modules with experimental-specifier-resolution', t => {
if (semver.satisfies(process.version, '<12.17'))
return t.skip('experimental-specifier-resolution requires node >= 12.17');
if (process.platform === 'win32') return t.skip('ESM support on windows is broken');

spawn('--experimental-specifier-resolution=node resolution.mjs', out => {
if (out.match(/touch message.js/)) {
Expand All @@ -22,8 +21,6 @@ tap.test('Supports ECMAScript modules with experimental-specifier-resolution', t
});

tap.test('Supports ECMAScript modules', t => {
if (process.platform === 'win32') return t.skip('ESM support on windows is broken');

spawn('ecma-script-modules.mjs', out => {
if (out.match(/touch message.mjs/)) {
touchFile('message.mjs');
Expand All @@ -38,8 +35,6 @@ tap.test('Supports ECMAScript modules', t => {
});

tap.test('Supports ECMAScript module packages', t => {
if (process.platform === 'win32') return t.skip('ESM support on windows is broken');

spawn('ecma-script-module-package/index.js', out => {
if (out.match(/touch ecma-script-module-package\/message.js/)) {
touchFile('ecma-script-module-package/message.js');
Expand All @@ -54,8 +49,6 @@ tap.test('Supports ECMAScript module packages', t => {
});

tap.test('We can hide the experimental warning by passing --no-warnings', t => {
if (process.platform === 'win32') return t.skip('ESM support on windows is broken');

spawn('--no-warnings ecma-script-modules.mjs', out => {
if (out.match(/ExperimentalWarning/)) return t.fail('Should not log an ExperimentalWarning');

Expand Down