search

istanbuljs / append-transform

handle multiple require hooks
MIT License
21 stars 4 forks source link

Move to istanbul organization? #8

Closed jamestalmage closed 6 years ago

jamestalmage commented 8 years ago

This code started in nyc, and nyc is now a part of the istanbul organization: https://github.com/istanbuljs/nyc/pull/286

I think it fits better there.

novemberborn commented 8 years ago

Or just with you, maybe?

jamestalmage commented 8 years ago

Or just with you, maybe?

Yeah. @sindresorhus has asked me to move this back out of avajs. If the istanbul team does not think it is a good fit in that org, then I'll move it back to my personal account.

// @gotwarlost @bcoe

bcoe commented 8 years ago

@jamestalmage I was talking to @isaacs about where spawn-wrap, foreground-child, and signal-exit should live too, they're a grey area since I think some of these modules are also used by `tap

tldr; I think if the module is only being used by nyc currently, perhaps move it into istanbuljs, and we can deprecate the other require-hook module if @gotwarlost is comfortable with this -- seems like we only need one require-hook module.

jamestalmage commented 8 years ago

It's two dependents are nyc and istanbul-lib-hook, so yeah - I think append-transform probably belongs there.

jamestalmage commented 8 years ago

I just tried to transfer, got: You don’t have admin rights to istanbuljs

sindresorhus commented 8 years ago

@jamestalmage Only org admins can transfer in repos. Just transfer it to an admin and they can transfer it into the org.

bcoe commented 8 years ago

@gotwarlost would you be comfortable giving some of the core nyc contributors access to the new istanbuljs org?

includes: @jamestalmage, @novemberborn, @sindresorhus; I created a team for them.

gotwarlost commented 8 years ago

@bcoe - sure, go ahead.

bcoe commented 8 years ago

@jamestalmage, @novemberborn you should now be owners 👍

gotwarlost commented 8 years ago

On the subject of killing istanbul-lib-hook, we need to make sure that nyc supports hooking vm.runInContext, vm.createScript etc. The first is needed for supporting require.js modules and the second was a feature someone asked for (don't remember details). Also, we need to expose flags to allow people to specify this.

I think I'll go through all of istanbul's tests and create a "list of features" doc so we can figure out if nyc already supports it, if it is a gap or if it can simply be dropped.

gotwarlost commented 8 years ago

Also, some thinking needs to go into how we support source-maps: contenders being remap-istanbul and istanbul-lib-source-map (this "works" AFAIK but has zero tests).

jamestalmage commented 8 years ago

I don't think there is any need to kill istanbul-lib-hook. append-transform is a general purpose library for attaching transforms, with the special caveat that your transform always goes last. It performs no transforms on it's own. It is used by istanbul-lib-hook/nyc to ensure their hook always comes after other transforms. This is less important for ES6 code now that istanbul is ES6 aware, but still important for coffeescript / typescript.

istanbul-lib-hook is a concrete require hook that performs the actual coverage transform.

NYC used append-transform to apply istanbul as a require hook, but also to do some source-map merging istanbul did not support at the time. This was a band-aid for ES6 support.

I think a likely solution is to port some of the source-map stuff to istanbul-lib-hook. It will be much easier to test the require hooks in isolated, single process environments before we add the complexities of NYC's multi-process spawn-wrap. NYC should just be responsible for installing that hook in every forked process. Tests for NYC are painfully slow because we need to fork a new process for basically every single one. The less logic in NYC itself, the better.