GeezerVison

@guybedford what might it look like to extend on the hooks available, such that a modified source-file could be stored in the cache?

I'm interested in how we'll continue providing test-coverage for .mjs files (which I believe circumvent the current require.extensions API -- this seems like it could be a step in the direction of what I'd need to get Istnabul working with .mjs?

@bcoe one option might be to add a separate 'fetch' hook to such a loader object alongside the 'resolve' here which could then add instrumentation. Happy to prototype it if there's interest.

@guybedford I'm very interested. The built-in coverage in V8 doesn't seem to quite be over the finish line yet, so this would be a great transitional step for continuing to support test-coverage with Istanbul.

@guybedford @bmeck any chance we could also allow you to attach a loader in a way similar to module.extensions. Perhaps module.loader? This would make the implementation way easier for tools like Istanbul. Then I could simply check whether module.loader exists, and instrument code accordingly.

@bcoe something like module.loaderHooks = { fetch, resolve } or module.registerHook('resolve', resolve) is possible, but there is the async load timing issue here in that the boot of the application will not run through the hooks in that case. The boot for such a scenario then might look like:

import module from 'module';
import { customResolve, customFetch } from 'custom-loader';

module.registerHook('resolve', customResolve);
module.registerHook('fetch', customFetch);

// now kick off the dynamic import hooking the main application tree load
import('./entry-point.mjs');

This was actually roughly what I had for the original API here, but after discussion with @bmeck we adopted a flag rather here to avoid some of the above issues. I'm still open to either approach.

Awesome to see this @guybedford ... left a few initial review comments but will need to play with this a bit more before signing off. Thank you for working on this

I'd like to leave things as declarative instead of API based if possible for now. require.extensions and people monkey patching module.* have cause a lot of lock-in that we cannot undo in CJS in part because of how hard it is to tell when something is being used, and in part because of the power of making them JS object properties.

As for a fetch transform, I have several ideas on how to accomplish similar ideas without introducing that transform:

1. make a file on disk w/ mutated source and resolve to that.
2. we introduce blob backed URL.createObjectURL and use that during resolution.

These can be worked through in the browser if done carefully; I am not convinced we should introduce fetch hooks in Node without creating a story for network backed modules and security of them. I also would like some involvement from browser people who are creating multiple different execution workers like ServiceWorker in order to do these tasks.

For code coverage I know V8 now has that instrumented in the codegen, so perhaps using that and adding missing features would be easier over time than trying to have hooks. Especially since we don't have a composition mechanism for these hooks currently.

This PR looks good even though I think we have more than enough hook area already and should not expand the scope of the PR. Overall, I think minimal hooks is the best approach in the beginning as I still think these hooks will change as we use them / won't look exactly the same by the time they are released.

I'm not sure when this is targeted to land, but this seems very early. I would hope this doesn't end up in Node core at least until the ecosystem has had some time to settle down and explore alternatives and prove that this particular API shape is the best, and that the use cases it addresses are actually important enough to add such a powerful, fragmentation-encouraging primitive which cannot be taken back.

General API comment: please never return or accept URL objects; see https://url.spec.whatwg.org/#url-apis-elsewhere .

On the actual capabilities here, I think the resolve hook is straightforward enough (although may end up mismatching browsers if they need something more declarative, as is currently the thinking). The loader object is much scarier and is nowhere near what we would currently contemplate exposing in browsers (as essentially we are trying to stick with Source Text Module Records, so any hook I can imagine would be just a fetch hook, i.e. service workers).

Thanks so much for all feedback so far. I've updated the description above to match the latest updates:

• I've renamed loader to format in the resolver hook, which is now fully consistent with being interpreted as the module format of the module being loaded, and no longer accepts a loader function. Hopefully this simplifies the resolve hook and makes it easier to see it as something that might be compatible with browsers too.
• To support the previous loader option, when the "dynamic" format is returned, module loading is delegated to a separate dynamicInstantiate hook which takes the place of the functional loader option.
• The hooks no longer accept or return URL objects, but work with string URLs only.

Ok, so @bcoe asked me to chime in:

My primary use case for a custom loader is mocks. I currently maintain require-inject which fiddles with the cache to allow tests to replace modules with their own code. How it does this is not something I'd want to see replicated as it's very much a hack, but the functionality is something I don't want to lose with ESM.

It looks like a loader API + Module.import() or a v8 import() would meet my needs.

An interpreter flag might be workable (because these are tests) but would be awkward, and some version of import() would still be needed. (Currently I can run my tests with node ./path/to/test.js, having to run it as node --with-loader=./path/to/loader.js ./path/to/test.mjs would basically make that kind of ad hoc command line use too awkward to use.)

@iarna

having to run it as node --with-loader=./path/to/loader.js ./path/to/test.mjs would basically make that kind of ad hoc command line use too awkward to use.)

I'd assume --loader package-name works too, which is similar to how I'm seeing folks use @std/esm (using node -r @std/esm ./path/to/test.js). Would that smooth things over?

@jdalton @iarna

I'd assume --loader package-name works too, which is similar to how I'm seeing folks use @std/esm (using node -r @std/esm ./path/to/test.js). Would that smooth things over?

This becomes a bit awkward for writing testing tools like nyc. Users will often spawn a subprocess in a cwd that potentially has its own node_modules folder. So, unless you were going to require that someone install this category of tool globally (which feels gross), you end up doing a lot of bookkeeping to ensure that spawned subprocesses have the appropriate flags.

This is a similar concern I have with trying to facilitate test-coverage functionality using V8's built-in coverage. Which is even more of a Rube Goldberg machine, as you need to spawn a process with --inspect, track which port has been used, etc.

tldr; it's a lot easier if there's a programatic way hook into this functionality in a vanilla Node process .

Users will often spawn a subprocess in a cwd that potentially has its own node_modules folder. So, unless you were going to require that someone install this category of tool globally (which feels gross)

I don't have a ton of experience with all that. I know that using tools like AVA, which forks processes, I was able to enable @std/esm by tying into their require option. Which is to say if AVA can track requires (and I'd guess flags too) couldn't nyc and others too?

@jdalton

I don't have a ton of experience with all that. I know that using tools like AVA, which forks processes, I was able to enable @std/esm by tying into their require option (which is serialized as a JSON string process.argv arg). Which is to say if AVA can do it couldn't nyc and others too?

nyc already does a lot of this, passing around state so that subprocesses can when spawned have their require.extensions patched (so that including a source-file results in it being instrumented by Istanbul).

My concern is that this flow is already very complex and bug prone. As it stands right now the new module system makes writing a development tool like this even more difficult.

EDIT: tldr; it feels like there's an opportunity to make the API for building features like this less painful, since the loader implementation is a greenfield right now. So, I don't see it as a question of whether it's possible, it's a question of can we reduce the burden placed on folks developing tools for Node.js.

@bcoe

This becomes a bit awkward for writing testing tools like nyc. Users will often spawn a subprocess in a cwd that potentially has its own node_modules folder. So, unless you were going to require that someone install this category of tool globally (which feels gross), you end up doing a lot of bookkeeping to ensure that spawned subprocesses have the appropriate flags.

You can use the NODE_OPTIONS environment variable to intercept the subprocess and attach a --loader argument. Is that sufficient?

This is a similar concern I have with trying to facilitate test-coverage functionality using V8's built-in coverage. Which is even more of a Rube Goldberg machine, as you need to spawn a process with --inspect, track which port has been used, etc.

This seems like we can improve that workflow rather than making a more complex hook system.

My concern is that this flow is already very complex and bug prone. As it stands right now the new module system makes writing a development tool like this even more difficult.

This is exactly why I want to work on a minimal API and iterate rather than trying to recreate the old workflows. If we focus on specific aspects instead of trying to solve all the use cases at once I think we will not suffer the previous problems with standardizing loaders.

@bmeck

That's a really thoughtful point of view; mainly I just want to make sure we have examples of how we'd facilitate use-cases like dependency injection, coverage, and transpilation, before ES modules aren't behind a flag... I think if we can nail these three categories of common developer tools, we'd know the API changes are looking really healthy.

@bcoe Dependency injection won't work like in other languages where it can change over time due to idempotency of ESM for any given specifier. The spec does not allow things like:

await import('thingFactory') != await import('thingFactory');

You can pass in data via query params as needed and that will give you a clean local module map that can change what thingFactory is. Can't re-use the same module though.

I'm not going to block unflagging on transpilation as they can be done ahead of time. However, various workflows like needing a way to get coverage is certainly blocking. If that is done ahead of time and works I would not see it as a blocker.

Oh, wait, I'm totally wrong, it had CI, even passing CI. (I searched for CI: rather than just CI because I'm a chump.) Sorry about that.

OK, so wonder what changed to cause the rash of failures all of a sudden...

@bmeck where would be a good place to start (or continue) a discussion about createObjectURL; I think this would be the most promising short term solution for keeping nyc/Istanbul working as we start to move some of the Node core modules over to .mjs.

Talking with @refack in IRC. The issue is indeed with this change but CI didn't catch it because of a bug in Makefile that caused the test to be omitted on CI.

Most relevant bit:

Yeah. This can't work on the ARM

node/test/es-module/test-esm-addon.mjs

Line 5 in 537402f

import binding from '../addons/hello-world/build/Release/binding.node';

And is also build-order-sensitive on all other platforms

Ref: #16155 (comment)

Yeah. This can't work on the ARM
And is also build-order-sensitive on all other platforms

For reference the arm-fanned job runs the addons suite on a different machine than any of the JS tests, so there is no compilation of the hello-world addon before test/es-module/test-esm-addon.mjs is run. (and also on the other platforms it's only by chance that it is done before this test is run).
tl;dr is should be moved to test/addons/hello-world/

Proposed fix in #16174