Repository navigation
nyc support for ES modules #492
Description
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.
Activity
hybrist commentedon Mar 2, 2020
@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 intonycor implement a similar approach to it?//cc @bcoe
bmeck commentedon Mar 2, 2020
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 commentedon Mar 3, 2020
FWIW @coreyfarrell AVA itself no longer does this. Our Babel provider does, but it does not support ESM yet.
@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 commentedon Mar 3, 2020
@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.
coreyfarrell commentedon Mar 3, 2020
@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--loaderarguments providing transformSource hooks to be run in order this would accomplish the same thing as pirates.novemberborn commentedon Mar 3, 2020
@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 commentedon Mar 6, 2020
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 commentedon Mar 11, 2020
@bmeck thanks for working on this. According to your readme
transformSourceis 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-hookwhich 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 fromprocess.env.NYC_CONFIG(for exampleparserPluginsto be passed to the babel parser).Does anyone know of any other
transformSourcehooks that exist? Even unpublished / non-merged hooks would be useful so we can do practical/real world tests of transform composition.bmeck commentedon Mar 11, 2020
@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 commentedon Mar 11, 2020
There's this: https://nodejs.org/api/esm.html#esm_transpiler_loader
coreyfarrell commentedon Mar 18, 2020
@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
defaultTransformSourcethough 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 commentedon Mar 18, 2020
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 itstransformSourcehook for.coffeefiles, and defers to Node default behavior for all other specifiers.Yes: https://runkit.com/geoffreybooth/coffeescript-compiler-with-inline-source-map