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.

Sign up for GitHub

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: synchronise default path of ModuleLoader.load #57390

Closed
wants to merge 2 commits into from
Closed

esm: synchronise default path of ModuleLoader.load #57390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32e213a
esm: synchronise `ModuleLoader`
JakobJingleheimer Mar 9, 2025
9f14b39
!fixup: don't force `ModuleLoader.load` to be async (can via hooks)
JakobJingleheimer Mar 9, 2025
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
11 changes: 5 additions & 6 deletions lib/internal/modules/esm/load.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,12 @@ const {
* @param {ESModuleContext} context used to decorate error messages
* @returns {Promise<{ responseURL: string, source: string | BufferView }>}
*/
async function getSource(url, context) {
function getSource(url, context) {
const { protocol, href } = url;
const responseURL = href;
let source;
if (protocol === 'file:') {
const { readFile: readFileAsync } = require('internal/fs/promises').exports;
source = await readFileAsync(url);
source = readFileSync(url);
Copy link
Member Author

Choose a reason for hiding this comment

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

This converts to a blocking op, but I'm not sure if that matters.

Copy link
Member

Choose a reason for hiding this comment

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

The CommonJS loadSource uses readFileSync:

node/lib/internal/modules/cjs/loader.js

Line 1128 in fbe37d5

return fs.readFileSync(filename, 'utf8');

though CommonJS is fundamentally sync. I think it should be fine to use the same in ESM; @joyeecheung or anyone else, do you have any concerns?

Copy link
Member

Choose a reason for hiding this comment

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

Blocking fs is generally not problematic when your fs is fast enough (and not taking on the async overhead might make it faster, as nodejs/performance#151 shows).

} else if (protocol === 'data:') {
const result = dataURLProcessor(url);
if (result === 'failure') {
Expand Down Expand Up @@ -80,7 +79,7 @@ function getSourceSync(url, context) {
* @param {LoadContext} context
* @returns {LoadReturn}
*/
async function defaultLoad(url, context = kEmptyObject) {
function defaultLoad(url, context = kEmptyObject) {
let responseURL = url;
let {
importAttributes,
Expand Down Expand Up @@ -110,13 +109,13 @@ async function defaultLoad(url, context = kEmptyObject) {
source = null;
} else if (format !== 'commonjs') {
if (source == null) {
({ responseURL, source } = await getSource(urlInstance, context));
({ responseURL, source } = getSource(urlInstance, context));
context = { __proto__: context, source };
}

if (format == null) {
// Now that we have the source for the module, run `defaultGetFormat` to detect its format.
format = await defaultGetFormat(urlInstance, context);
format = defaultGetFormat(urlInstance, context);
Copy link
Member Author

Choose a reason for hiding this comment

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

defaultGetFormat is already sync


if (format === 'commonjs') {
// For backward compatibility reasons, we need to discard the source in
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/modules/esm/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,9 +804,9 @@ class ModuleLoader {
* if any.
* @param {string} url The URL of the module to be loaded.
* @param {object} context Metadata about the module
* @returns {Promise<{ format: ModuleFormat, source: ModuleSource }>}
* @returns {Promise<{ format: ModuleFormat, source: ModuleSource }> | { format: ModuleFormat, source: ModuleSource }}
*/
async load(url, context) {
load(url, context) {
if (loadHooks.length) {
// Has module.registerHooks() hooks, use the synchronous variant that can handle both hooks.
return this.#loadSync(url, context);
Expand Down
Loading