module: expose resolveLoadAndCache API

[aduh95]

Opening as draft as I'd like to get some feedback on the API design first. /cc @nodejs/loaders

Use case for this is to get "final" format of the module that would result from importing a specific specifier. The cache part is primordial as otherwise you'd have no guarantee that the result would be accurate (if there's a user loader involved, we have no reason to believe it would be stable over time). However, caching a call that's sometimes async, sometimes sync is more or less impossible in JS, and on top of that there are loaders out there that rely on resolve NOT being cached (e.g the test runner .mock util), so instead I decided it'd be more useful to provide a way to skip the resolve call.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[JakobJingleheimer]

This is already doing that, no? I see gets and sets

[JakobJingleheimer]

Or, it appears to do for ModuleLoader but not CustomizedModuleLoader. Is that what you mean?

[legendecas]

When --experimental-strip-types is enabled and performing this operation on a .ts file, this format may contains commonjs-typescript and module-typescript which are not documented at https://nodejs.org/api/module.html#loadurl-context-nextload as final formats.

I think we should convert commonjs-typescript and module-typescript to commonjs and module respectively. Alternatively, document the other two and make them public.

[aduh95]

I think we probably want to document it, as it's certainly going to be helpful for the ecosystem to know which files are TypeScript

[codecov]

Codecov Report

Attention: Patch coverage is 89.58333% with 5 lines in your changes missing coverage. Please review.

Project coverage is 90.21%. Comparing base (c712dd2) to head (434cbf2).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/helpers.js	87.17%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55756      +/-   ##
==========================================
+ Coverage   89.66%   90.21%   +0.55%     
==========================================
  Files         630      630              
  Lines      186389   186437      +48     
  Branches    36286    36612     +326     
==========================================
+ Hits       167124   168198    +1074     
+ Misses      12049    11060     -989     
+ Partials     7216     7179      -37     

Files with missing lines	Coverage Δ
lib/module.js	100.00% <100.00%> (ø)
lib/internal/modules/helpers.js	97.87% <87.17%> (-0.05%)	⬇️

... and 100 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[legendecas]

Seems like unrelated changes.

[legendecas]

Not a fan of implicitly skip resolve when parentURL is undefined. This also contradicts with the API name resolve, load, and cache.

We have several conventions in APIs about how to resolve the parentURL:

• new Worker(filename) resolves with base to process.cwd(),
• module.register(specifier[, parentURL]) resolves with base to data: if parentURL is not set.

I think as part of module loader API, it is better to be explicit, e.g. throw if parentURL is missing.

[aduh95]

• module.register(specifier[, parentURL]) resolves with base to data: if parentURL is not set.

I think as part of module loader API, it is better to be explicit, e.g. throw if parentURL is missing.

Note that this way of doing things is consistent with module.register, i.e. it will throw if specifier is not a full URL and parentURL is missing (because load only accepts full URLs).

Not a fan of implicitly skip resolve when parentURL is undefined. This also contradicts with the API name resolve, load, and cache.

The name is not great, I'm certainly fine changing it to something else. IMO being able to skip resolve is important as it allows to get results guaranteed to be true for the lifetime of the process (at least until we expose a way to clear the load cache), I'm open to suggestions if you think we can make a clearer API

[legendecas]

What I'm concerned about is that using a full specifier URL and an implicit parentURL to indicate skipping resolve is not alike with import:

import 'test:./foobar'

With import statements or import calls, a resolve hook will be invoked with a full specifier URL and implicit parentURL.

export function resolve(specifier, context, next) {
  if (specifier.startsWith('test:')) {
    // Translate custom protocol and use the default loader.
    return next(specifier.slice(5), context);
  }
  return next(specifier, context);
}

[aduh95]

We cannot make it work like import, unless we define it on import.meta, but I get what you're saying. What do you think about going back to using data: as default parentURL, and expose some symbol to explicitly skip the resolve call if needed?

// does not go through `resolve` hook:
resolveLoadAndCache('full://module/url', module.SKIP_RESOLVE_CALL);
// or maybe
resolveLoadAndCache('full://module/url', Symbol.for('nodejs.skipResolveCall'));

// does go through `resolve` hook with `data:` as `parentURL`:
resolveLoadAndCache('full://module/url');
// does go through `resolve` hook just like `import`/`import()` would:
resolveLoadAndCache('full://module/url', import.meta.url);

[legendecas]

Yeah, I agree that this API does not infer parentURL implicitly to the caller module (it is not practical anyway). I think a possible alternative to Symbol based API is separating the motivation of invoking each of the loader hook into individual APIs:

interface ModuleLoaderAPI {
  resolve(specifier, parentURL, attributes): Promise<{ url, format }>;
  load(url, attributes): Promise<{ format, source }>;

  resolveAndLoad(specifier, parentURL, attributes): Promise<{ url, format, source }>;
}

In this way, the API intention is explicit.