esm: fix http(s) import via custom loader

[JakobJingleheimer]

When a custom loader is used to load a remote asset via http(s), the internal cache is circumvented. This results in a nested import's resolution to run into an unsettled promise (causing parentURL to be undefined instead of its actual value).

fixes #42721

TODO:

• Add test case(s) for this specific scenario (the current test cases do not cover nested imports).

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/modules

[JakobJingleheimer]

@guybedford I quickly looked into the ModuleJob avenue you suggested, but I'm pretty sure it will take longer than the next ~hour I have before being gone a week. To resolve this bug sooner rather than later, I say we go with this workable-but-not-quite-ideal fix and improve it in a follow-up (which I'd be happy to do when I get back, probably after switching resolve to synchronous?). I added an item to the Loaders project for it so we don't forget.

I updated the jsdocs and added some "there be dragons" code docs.

[GeoffreyBooth]

Please confirm that the tests fail without your lib/ changes.

[JakobJingleheimer]

I did (see #43130 (comment))

[guybedford]

@JakobJingleheimer I hope you don't mind I pushed a commit to add a fetch cache guard to avoid double fetching and clarify the contract.

As mentioned before the architectural fix should be:

• Allow the load hook to return a { responseUrl } as separate to the initial resolved URL, which is stored on the module job.
• Then instead of reading off from the fetch cache, rather read off from the stored module job responseUrl instead of this function (and then, is a fetch cache even needed?)

For now it would be great to get this fix merged certainly.

[aduh95]

A few optional nits, otherwise LGTM :)

[lmichailian]

Hi! When do you merge this PR or how can I help with this? thanks!

PD: There is some workaround ?

[JakobJingleheimer]

@guybedford I don't mind 🙂 thanks! Good improvement!

@aduh95 good nits, especially the test one. I can't apply them right now. Would you mind batch-committing them and add the label to merge? (GitHub mobile doesn't support it)

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/44152/

[nodejs-github-bot]

Landed in b5ed1bd

[bengl]

Hi @JakobJingleheimer. This doesn't land cleanly on v18.x. Could you submit a backport PR?

[targos]

I am not on my computer to link to it, but this depends on a previous loaders PR that has the backport blocked label

[JakobJingleheimer]

What @targos said (it's not supposed to yet). Sorry, should I apply a label or something?

[bengl]

@JakobJingleheimer I'll just change the label to backport blocked. If someone can throw the link in here to the previous PR once they find it, that would be great, so we can make sure that this one lands on a release when that one does.

[JakobJingleheimer]

#42623