Repository navigation
custom loader fails to resolve nested http(s) import #42721
Description
Activity
/cc @nodejs/loaders
Reacted by Jacob SmithDynamic 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.mjsis 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 usesesm/fetch_module). I think this code should not be hit as--experimental-network-importswas 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.
It should be possible to make a custom loader to do what
--experimental-network-importsdoes, both in principle and so that people have a way to achieve use cases that--experimental-network-importsexcludes (like supportinghttp:URLs, for example).Reacted by Jacob Smith- changed the title
[-]Dynamic import fails with custom loader[/-][+]http(s) import fails with custom loader[/+]on Apr 18, 2022 The problem occurs when the request is not made through network imports:
ModuleJob::linkedlooks up the cached value of the request, which, when network imports is circumvented, is an unsettled promise. It usesESMLoader::getBaseURL()to do that, and this error isESMLoader::getBaseURL()detecting the problem. I thinkESMLoader::getBaseURL()needs to stay synchronous (so it can'tawaitthe pending request) (cc @bmeck ?).ModuleJob can't
awaitESMLoader::getBaseURL()'s return becauseESMLoader::getBaseURL()needs to pluckresolvedHREFfrom the pre-settled promise's value.In my above example, the problem occurs when resolving
lodash.js's imports (parentURLshould behttps://…, but it's nothing because of the unsettled promise), and then hitsdefaultResolve().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 pluckresolvedHREF, that could maybe do it 🤔 That might spiral though.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 pluckresolvedHREF, 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
undefinedin "Base url for module ${url} not loaded." (urlwas previously getting overwritten before the error message is printed), and it instead includes the URL of the parent.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?
I think because of redirects?
I can verify this bug is breaking JSPM support for network imports with Node.js import maps. We can't use
--experimental-network-importsbecause packages import core modules from the network.It's pretty ironic that the PR that introduced network imports broke network imports. Is anyone working on a fix?
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.
@JakobJingleheimer if you're interested in trying out the case against the chaining PR, this is the loader that is broken - https://github.com/node-loader/node-loader-http.
haha thanks, but you know I wrote that, right? 😜
No, because you aren't attributed at all! Are you definitely sure the chaining PR will fix this issue? I can also put some time aside to test that if it would help.
No, because you aren't attributed at all!
Ah, I think maybe it got lost when the repo was moved into nodejs.
Are you definitely sure the chaining PR will fix this issue?
No, I don't think the chaining PR will fix this issue at all.
Once the chaining PR is in, I'll focus on fixing this :)
I can also put some time aside to test that if it would help.
I'll hit you up soon for it!
Ah, I think maybe it got lost when the repo was moved into nodejs.
I think you two are talking about different repos. My old repo that got moved into the
nodejsorg is https://github.com/nodejs/loaders-test. The one @guybedford is referring to is https://github.com/node-loader/node-loader-http, part of a separate org https://github.com/node-loader/.Reacted by Jacob SmithOnce the chaining PR is in, I'll focus on fixing this :)
Not sure I agree with the prioritization here, but thanks for keeping it on the radar!
The chaining PR is almost done.
Chaining PR has landed. Focusing on this now.
@GeoffreyBooth we didn't catch this because https://coffeescript.org/browser-compiler-modern/coffeescript.js (from nodejs/loaders-test/https-loader) is a bundle and has no imports of its own.
- changed the title
[-]http(s) import fails with custom loader[/-][+]custom loader fails to resolve nested http(s) import[/+]on May 4, 2022
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
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.
Additional information
No response