Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for v8 code coverage #8596
feat: add support for v8 code coverage #8596
Changes from 19 commits
86fd625
df460c7
14f8b2c
66a1e7f
587bea3
4cbe3e5
839bfab
00436c9
c80ef46
eec6fa2
b5ff1ab
aaec90d
4cc2232
718a1a1
af4482b
70ee018
c190ae6
6bade4a
98b4f49
cf34642
c994954
c2ff7d8
c5c1de6
78ceba8
881fd82
9c7c353
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this module does not (and should not) have to live here. Maybe we can make it part of
istanbul-lib-instrument
? All it does is start and stop the coverage collection using the v8 inspector API./cc @bcoe @coreyfarrell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have time to really look at this currently but I wouldn't object to creating a new module under the istanbuljs umbrella to accomplish this. Maybe
@istanbuljs/v8-coverage
would be a good name? I think it should be a separate module fromistanbul-lib-instrument
which pulls in a bunch of babel modules.FWIW under the istanbuljs org I would not want TS source code but I don't have a problem with having the module include an
index.d.ts
with public definitions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I honestly have less concerns about TypeScript, since I've been using it at work these days; would be happy to put up my hand and help take on maintenance burden for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to take the code from this. Or create the repo and I can push it there.
If you want a separate
d.ts.
file, here's the built definition:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I stuck it in https://github.com/SimenB/collect-v8-coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lost type info if I used
util.promisify
, so I just wrapped everything in promises manuallyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting problem to solve. It's possible to strongly type the function returned by
util.promisify
by using some of the techniques described in this issue or this SO answer (kudos to their authors btw, ingenious ideas).I tried this myself after reading both of the links above and managed to get it working this way:
Create a conditional type which will be the union type of the variadic arguments.
Add type aliases for the
Callback
s and thepromisify
function itself.PromisifyOne
is just a conditional type which depends on the number of arguments passed (as they're variadic) and theCallback
's generic type forreply
.This makes sure that the return type will have the same type as the
Callback
s second argument.Use the type of
util.promisify
's argument to obtain it's return type.With the tree snippets above you can get the type info for
func
correctly in the example below:I'm not sure why the other examples had to use
UnionToIntersection
to achieve that so I might have missed something there.I'm also not sure if the tradeoff between having these complex type definitions versus manually wrapping these in promises is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into it! I don't think it's worth the complexity here, but maybe you could upstream those changes to DT? The definitions are here: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/796b838a15fad73287bad7a88707a9ca04e60640/types/node/util.d.ts#L86-L105
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely agree, I think this actually belongs to DT's codebase since it's essentially covering what the type defs should be there.
I'll have a look at that, thanks @SimenB 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could move this to "Istanbul/merge-v8-coverage" or "merge-v8-coverage", @demurgos do you have a preference? Would you like to work on this library within the Istanbul organization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
As discussed in the
v8-coverage
repo, I am working on a 1.0.0 release of the lib. I agree that it would make sense to move it the istanbul org.