custom loader fails to resolve nested http(s) import

[basmasking]

Version

v17.9.0

Platform

Darwin MacBook-Pro.local 21.4.0 Darwin Kernel Version 21.4.0: Fri Mar 18 00:45:05 PDT 2022; root:xnu-8020.101.4~15/RELEASE_X86_64 x86_64

Subsystem

No response

What steps will reproduce the bug?

Use a custom loader based on the http loader example
Start node with the --experimental-loader flag.
Try to load any external esm module file via https using the following function

import('https://my-domain.com/my-module.js'); 

How often does it reproduce? Is there a required condition?

Always from version v17.7.0 and onwards

What is the expected behavior?

To load and import the external module

What do you see instead?

The data is loaded in the custom loader as a string and gets resolved as the loader example. Then the following error is thrown.

Error [ERR_INTERNAL_ASSERTION]: Base url for module undefined not loaded.
This is caused by either a bug in Node.js or incorrect usage of Node.js internals.
Please open an issue with this stack trace at https://github.com/nodejs/node/issues
    at new NodeError (node:internal/errors:372:5)
    at ESMLoader.getBaseURL (node:internal/modules/esm/loader:290:15)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:79:37)
    at link (node:internal/modules/esm/module_job:78:36)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  code: 'ERR_INTERNAL_ASSERTION'
}

Additional information

No response

[aduh95]

/cc @nodejs/loaders

[JakobJingleheimer]

Dynamic import appears to be a red herring—I get the same error for a regular import:

node \
  --experimental-loader=/…/https-loader.mjs \
  --input-type=module \
  -e "import lodash from 'https://unpkg.com/lodash-es@4.17.21/lodash.js'; console.log(lodash)"

My https-loader.mjs is a copy-paste of the example in the docs.

The error thrown is from ESMLoader::getBaseURL(), which is part of experimental network imports (and subsequently uses esm/fetch_module). I think this code should not be hit as --experimental-network-imports was not supplied 🤔

I think the introduction of network imports broke this example, causing ESMLoader to try to use pieces of itself that haven't been hydrated as it expects (because the code went through a different journey).

Switching from a custom loader to a network import does work:

node \
  --experimental-network-imports \
  --input-type=module \
  -e "import('https://unpkg.com/lodash-es@4.17.21/lodash.js').then(console.log)"

I'll need to dig into this a little more to see if this is worth fixing, or if we should just tell people to use a network import.

[GeoffreyBooth]

It should be possible to make a custom loader to do what --experimental-network-imports does, both in principle and so that people have a way to achieve use cases that --experimental-network-imports excludes (like supporting http: URLs, for example).

[JakobJingleheimer]

The problem occurs when the request is not made through network imports: ModuleJob::linked looks up the cached value of the request, which, when network imports is circumvented, is an unsettled promise. It uses ESMLoader::getBaseURL() to do that, and this error is ESMLoader::getBaseURL() detecting the problem. I think ESMLoader::getBaseURL() needs to stay synchronous (so it can't await the pending request) (cc @bmeck ?).

ModuleJob can't await ESMLoader::getBaseURL()'s return because ESMLoader::getBaseURL() needs to pluck resolvedHREF from the pre-settled promise's value.

In my above example, the problem occurs when resolving lodash.js's imports (parentURL should be https://…, but it's nothing because of the unsettled promise), and then hits defaultResolve().

On the surface, this seems like a catch-22. I'll try to look more into this later this week.

EDIT: Perhaps if we only throw in ESMLoader::getBaseURL() when the network imports flag is set and then anywhere that would receive the unsettled promise awaits it and themselves pluck resolvedHREF, that could maybe do it 🤔 That might spiral though.

[JakobJingleheimer]

EDIT: Perhaps if we only throw in ESMLoader::getBaseURL() when the network imports flag is set and then anywhere that would receive the unsettled promise awaits it and themselves pluck resolvedHREF, that could maybe do it 🤔 That might spiral though.

esm/module_job.js

      const promises = this.module.link(async (specifier, assertions) => {
        const base = await this.loader.getBaseURL(url);
        const baseURL = typeof base === 'string' ?
          base :
          base.resolvedHREF;

        const jobPromise = this.loader.getModuleJob(specifier, baseURL, assertions);
        ArrayPrototypePush(dependencyJobs, jobPromise);
        const job = await jobPromise;
        return job.modulePromise;
      });

esm/loader.js

  getBaseURL(url) {
    if (
      StringPrototypeStartsWith(url, 'http:') ||
      StringPrototypeStartsWith(url, 'https:')
    ) {
      // The request & response have already settled, so they are in
      // fetchModule's cache, in which case, fetchModule returns
      // immediately and synchronously
      const module = fetchModule(new URL(url), { parentURL: url });

      if (typeof module?.resolvedHREF === 'string') { // [2]
        url = module.resolvedHREF;
      } else { // This should only occur if the module hasn't been fetched yet
        if (getOptionValue('--experimental-network-imports')) {
          throw new ERR_INTERNAL_ASSERTION(
            `Base url for module ${url} not loaded.`
          );
        } else {
          url = module;
        }
      }
    }

    return url;
  }

A quick test suggests this route is viable and continues to work when using network imports instead.

@bmeck @GeoffreyBooth thoughts?

A fringe-benefit is the error message no-longer cites undefined in "Base url for module ${url} not loaded." (url was previously getting overwritten before the error message is printed), and it instead includes the URL of the parent.

[GeoffreyBooth]

I haven't dug into the code, but can you explain why the base URL isn't known while the promise is unsettled? Like don't we know what URL we're in the process of resolving, and therefore we also know its base URL?

[JakobJingleheimer]

I think because of redirects?

[guybedford]

I can verify this bug is breaking JSPM support for network imports with Node.js import maps. We can't use --experimental-network-imports because packages import core modules from the network.

[guybedford]

It's pretty ironic that the PR that introduced network imports broke network imports. Is anyone working on a fix?

[JakobJingleheimer]

C'est moi. Loader chaining is inches from done and the fix I outlined above touches the same piece that appears to be blocking Loader chaining; I've been trying to investigate the issue blocking loader chaining for a few weeks now and reaallly don't want to flip the table on that and have to start all over.

[guybedford]

@JakobJingleheimer is that the async hooks thing? Note that the Node.js bootstrap cannot be altered in terms of timing... I lost a lot of cycles to that as well in the past.

[JakobJingleheimer]

Yes, it's the async_hooks thing, and I'm super baffled cuz everything looks exactly like it should.

If that is it, I don't understand why that would be affected by my change: my change only affects things when multiple custom loaders are supplied. The existing code that handles regular imports isn't changed.