Skip to content
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: Node API #19

Closed
wants to merge 1 commit into from
Closed

Conversation

laggingreflex
Copy link
Contributor

This commit attempts to refactor the code to separate the running/reporting/writing logic so that it can be consumed by other libraries

@laggingreflex laggingreflex force-pushed the feat/node-api branch 2 times, most recently from 8b6dff4 to 06f3629 Compare August 4, 2018 15:34
This commit attempts to refactor the code to separate the running/reporting/writing logic so that it can be consumed by other libraries
@demurgos
Copy link
Contributor

demurgos commented Aug 4, 2018

😄
After many months without any commit, it seems there are two of us working on this project. Following the invalid test results, I refactored both v8-to-istanbul and c8. For the moment I was just fixing bugs, but I agree that a Node API would be very valuable.
When I'll finish fixing the bugs, I'll take a look at your PR and leave review comments. If it's good, I'll rebase it on top of my changes.

@demurgos
Copy link
Contributor

demurgos commented Aug 6, 2018

I had some time to look at your proposal.
I think that this is the API we should strive for in the long term, but I don't think it is currently safe to provide this kind of API.
What I mean is that it'd be great to just pass a function and then get coverage results for it, but it does not work well with the isolation of the executions. c8 has a client/server architecture where the server wraps all the node processes in its process sub-tree. Then each node invocation starts a client. The client bootstraps the profiler, delegates the execution to the original program and then return the results to the server by writing them in a temporary file. When the sub-process tree ends, the server reads the results from the files left by all the clients and generates the reports.

From what I see, your current API proposal is intended to be executed at the client level. It has a few issues:

  • Spawned sub-processes are not covered.
  • You cannot call your API multiple times: the coverage is global per process, not "scoped" to a single instrumented function call.
  • Even if you were to be able to call your API multiple times (by manually handling scope issues), there would be concurrency concerns (for example regarding module caching).
  • This API is not the Node equivalent of the CLI: the CLI works with sub-processes.

Providing helpers for reporters is fine. There's probably some bike-shedding if it should return V8 reports and let the consumer convert them to Istanbul's format or not. (So c8 would be less reliant on Istanbul and could be used by other kinds of reporters.)

I'd start with a more conservative API with similar capabilities as the CLI (so the CLI is just a wrapper of the Node API). The instrumented code would still have to be executed in its own sub-process. What I would provide is a function with an API similar to child_process.spawn but instead of returning a ChildProcess, it would return a promise for coverage reports collected by executing the corresponding command.

@bcoe
Copy link
Owner

bcoe commented Aug 11, 2018

@demurgos @laggingreflex 👋 sorry for not having helped steward this project in recent months.

This project started out mainly as a proof of concept, to prove out whether or not we could potentially use v8's native test coverage for Node.js projects. Well things work relatively well, I ultimately landed on the opinion that we'd do better to try to integrate things more tightly into Node.js itself.

I'm hoping to continue working with @hashseed @schuay to better define what this would look like.

If you're interested in joining this conversation, or want to bounce any ideas off of me related to c8, I can be found in chat here:

http://devtoolscommunity.herokuapp.com/

@bcoe bcoe closed this Sep 8, 2018
@bcoe
Copy link
Owner

bcoe commented Sep 8, 2018

@laggingreflex see #22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants