nyc support for ES modules

[coreyfarrell]

I'm trying to determine the best user experience of nyc supporting ES module loader hooks and I'm troubled by node.js not supporting multiple hooks. A substantial number of our users combine nyc with other transformations, ts-node/register being a common example.

A common workflow is for users to run nyc --require ts-node/register mocha or nyc mocha --require ts-node/register mocha. In addition some test runners such as ava or tap which might automatically enable certain transformations for the user.

One potential idea is that nyc could just not handle instrumentation of ES modules. We could create an @istanbuljs/esm-transform module and leave it to the end-user to enable the plugin. I'm concerned this will be a user experience regression compared to how nyc is commonly plug-n-play for CJS, also concerned about the complexity needed to document this.

I acknowledge that complex loader hook composition would need to be done in user-space but it would be very nice for node.js to natively facilitate the most basic 'stack of hooks' use case.

@nodejs/tooling @isaacs @novemberborn any comments from the view of test runners would be useful.

[jkrems]

@coreyfarrell How does this relate to things like c8? My impression was that a clean way for supporting coverage for modules would be to use the V8 coverage APIs. Would it be possible to integrate it into nyc or implement a similar approach to it?

//cc @bcoe

[bmeck]

It might be prudent to release a user-land compositional loader for experimentation. I think we should consider this if we are facing opposition to composition within node's core to at least help organize community efforts.

[novemberborn]

In addition some test runners such as ava

FWIW @coreyfarrell AVA itself no longer does this. Our Babel provider does, but it does not support ESM yet.

It might be prudent to release a user-land compositional loader for experimentation.

@bmeck, even for CJS, registering multiple loaders is sufficiently complicated that user-land solutions like https://www.npmjs.com/package/pirates have come about.

[bmeck]

@novemberborn

@bmeck, even for CJS, registering multiple loaders is sufficiently complicated that user-land solutions like https://www.npmjs.com/package/pirates have come about.

Is this an endorsement or an argument against creating compositional loaders? CJS was never designed to be compositional and doesn't have isolation or guarantees about a lot of the ways people are mutating (often undocumented) internals to accomplish things.

[coreyfarrell]

@jkrems I don't think it's possible to integrate c8 or v8 native coverage into nyc. @bcoe has done great work on c8 and I use it in a couple projects but there are cases where the coverage results produced by nyc are preferable. Users can choose to use c8 instead of nyc but I think it's important that ES modules do not eliminate the choice of nyc. A specific concern I have is that c8 coverage issues sometimes require a fix to v8 in C++. This significantly restricts the number of people with the ability to contribute fixes and lengthens time for fixes to reach a node.js release, especially if the v8 patch cannot be backported. Over time as v8 native coverage becomes more battle tested this will become less of an issue.

@bmeck I understand what you're suggesting but haven't had much success thinking of how this in concrete ways (UX/API/feature planning). I am still keeping this in mind and trying to think of how it would come together.

pirates is used because CJS does not actually provide any hook for transformations CJS transformations require patching node.js internals. For example patching Module._extensions['.js'] multiple times would be problematic. If node.js supported multiple --loader arguments providing transformSource hooks to be run in order this would accomplish the same thing as pirates.

[novemberborn]

Is this an endorsement or an argument against creating compositional loaders? CJS was never designed to be compositional and doesn't have isolation or guarantees about a lot of the ways people are mutating (often undocumented) internals to accomplish things.

@bmeck I'd prefer this to be built-in, but user-land has shown it can converge on a solution for this if necessary.

[bmeck]

I've thrown together https://github.com/bmeck/compositional-esm-loader for now to play around with ideas we could have. While creating that, I did notice that users can disable loaders which I'm not sure we want to actually allow.

[coreyfarrell]

@bmeck thanks for working on this. According to your readme transformSource is disabled, once that is not the case I can probably do some testing with your module (I haven't had a chance to actually look at your code so I'm not sure what the issue is). I'm working on creating @istanbuljs/esm-loader-hook which will closely resemble https://github.com/tapjs/libtap/blob/master/istanbul-loader-hook.mjs, will just need a bit of extra code for detecting certain options from process.env.NYC_CONFIG (for example parserPlugins to be passed to the babel parser).

Does anyone know of any other transformSource hooks that exist? Even unpublished / non-merged hooks would be useful so we can do practical/real world tests of transform composition.

[bmeck]

@coreyfarrell it is actually enabled right now, but is doing something hacky to buffer return values. The issue is from how we stringify Module source text using .toString and if you pass it a TypedArray it gets angry. I'm unclear on where I'd want to fix the bug, either in the userland loader or core, but I'm slowly thinking core needs a more concrete explanation of how types are converted into strings etc. for the various translators.

[GeoffreyBooth]

Does anyone know of any other transformSource hooks that exist?

There's this: https://nodejs.org/api/esm.html#esm_transpiler_loader

[coreyfarrell]

@bmeck I haven't had a chance yet to experiment with your compositional loader but I did finally get around to publishing https://github.com/istanbuljs/esm-loader-hook so we have something concrete which shows what istanbuljs hook needs to accomplish. I suspect I need to alter it to call defaultTransformSource though I'm not completely clear on the purpose of the transformSource default function.

@GeoffreyBooth this is neat I'll have to try it out once I have more time. If I understand correctly I'll need to modify the example to have CoffeeScript.compile(source, { bare: true, inlineMap: true }) to produce inline source-maps?

[GeoffreyBooth]

I'm not completely clear on the purpose of the transformSource default function.

The purpose of all of the default* functions is to let you write a hook that only does custom logic for some specifiers, letting Node do its normal thing for the rest. So for example the CoffeeScript loader does custom stuff in its transformSource hook for .coffee files, and defers to Node default behavior for all other specifiers.

If I understand correctly I'll need to modify the example to have CoffeeScript.compile(source, { bare: true, inlineMap: true }) to produce inline source-maps?

Yes: https://runkit.com/geoffreybooth/coffeescript-compiler-with-inline-source-map