Tracking Issue: Syncify the ESM Loader

Question

Tracking Issue: Syncify the ESM Loader

GeoffreyBooth opened this issue a year ago · 10 comments

Answer 1 · 2024-11-08T14:11:12.000Z

Maybe moving it in the loaders repo would be more appropriate since its a effort that is gonna require different iterations?

Answer 2 · 2024-11-08T15:10:50.000Z

This task is better handled by a large group of people, and this repo is more visible. Once the first few steps are done, it can easily be tagged as "good first issue"

Answer 3 · 2024-11-10T15:52:45.000Z

I would like to mention my issue here.
Please check if it's worth fixing.

Answer 4 · 2024-11-16T12:25:17.000Z

Hi @mcollina and @marco-ippolito,
Is anyone currently working on any of these issues? I’d love to help resolve a few. Could you recommend a good starting point? I’d like to begin with something low-risk—what would be the best issue to pick?

Also what is the best method to test if my code is improving performance?

Answer 5 · 2025-02-18T15:08:51.000Z

This may be necessary to prevent a race condition like prettier/prettier#17139 (comment)

Answer 6 · 2025-03-09T23:32:15.000Z

@cu8code Sorry to not notice your comment earlier. If you'd like to help with this effort, you can pick any of the functions listed in the top issue and try to refactor them to be synchronous. The lowest hanging fruit are the ones that are async functions but never actually await anything (perhaps because they used to contain async logic but no longer do, for example). Besides prioritizing the easiest parts, I would also prioritize the hottest code paths: the functions involved in initial load of the most common use case like node app.js where app.js is an ES module. I would make the PRs as small as possible so that any resulting issues can be isolated as narrowly as possible, along the lines of #57390.

Answer 7 · 2025-03-17T12:01:03.000Z

esm/fetch_module.js: fetchWithRedirects
esm/fetch_module.js: isLocalAddress

Can be removed since we removed http imports

Answer 8 · 2025-03-17T22:18:18.000Z

re: esm/load.js getSource

is there any reason the calling functions can't call getSourceSync (or even the whole function replaced with getSourceSync everywhere it's called?

/**
 * @param {URL} url URL to the module
 * @param {ESModuleContext} context used to decorate error messages
 * @returns {Promise<{ responseURL: string, source: string | BufferView }>}
 */
 async 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);
  } else if (protocol === 'data:') {
    const result = dataURLProcessor(url);
    if (result === 'failure') {
      throw new ERR_INVALID_URL(responseURL, null);
    }
    source = BufferFrom(result.body);
  } else {
    const supportedSchemes = ['file', 'data'];
    throw new ERR_UNSUPPORTED_ESM_URL_SCHEME(url, supportedSchemes);
  }
  return { __proto__: null, responseURL, source };
}

 /**
 * @param {URL} url URL to the module
 * @param {ESModuleContext} context used to decorate error messages
 * @returns {{ responseURL: string, source: string | BufferView }}
 */
 function getSourceSync(url, context) {
  const { protocol, href } = url;
  const responseURL = href;
  let source;
  if (protocol === 'file:') {
    source = readFileSync(url);
  } else if (protocol === 'data:') {
    const result = dataURLProcessor(url);
    if (result === 'failure') {
      throw new ERR_INVALID_URL(responseURL);
    }
    source = BufferFrom(result.body);
  } else {
    const supportedSchemes = ['file', 'data'];
    throw new ERR_UNSUPPORTED_ESM_URL_SCHEME(url, supportedSchemes);
  }
  return { __proto__: null, responseURL, source };
}

Answer 9 · 2025-04-02T12:33:13.000Z

@ntindle no, there isn't 👍 getSource and getSourceSync were deduplicated in the PR(s) that syncify defaultLoad. I'm gonna collapse that comment and this as resolved in the interest of keeping this issue easy to grok.

Answer 10 · 2025-04-04T18:55:04.000Z

#57749 adds everysync as decided in the collab summit.

The files to be updated, all underlib/internal/modules:

The files to be updated, all under`lib/internal/modules`: