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

fun: switch to async IO #9

Closed
bcoe opened this issue Dec 5, 2017 · 6 comments
Closed

fun: switch to async IO #9

bcoe opened this issue Dec 5, 2017 · 6 comments
Labels
good first issue hackillinois help wanted Issue is well defined but needs assignment

Comments

@bcoe
Copy link
Owner

bcoe commented Dec 5, 2017

c8 already requires a bleeding edge v8 engine, we should switch to async/await rather than using any synchronous IO.

see @TimothyGu's comments in #8.

@bcoe bcoe added good first issue help wanted Issue is well defined but needs assignment labels Dec 5, 2017
@Bnaya
Copy link

Bnaya commented Feb 7, 2018

@bcoe what do you think about typescript? :)
I would love to help refactor the codebase to typescript + utilising async functions

@bcoe
Copy link
Owner Author

bcoe commented Feb 18, 2018

@Bnaya 👋I'm not a huge advocate of Typescript; part of why I'm excited about native coverage support in v8 is that it can eliminate some of the transpilation that is currently being performed by Istanbul.

But! I would love your help on c8; if you'd like to contribute, strike up a conversation with me in this Slack:

http://devtoolscommunity.herokuapp.com/

@profnandaa
Copy link
Contributor

profnandaa commented Sep 17, 2018

@bcoe - would like to take up this one (for fun :)). I can't seem to get the code that is being referred here:

see @TimothyGu's comments in #8.

in bin/c8.js, do we rewrite rimraf (has Async API) and mkdirp (no async, so means rewriting with fs.mkdir going for the new fs.mkdir)?

Do we also change the fs.*Sync's in /lib/parse-args.js and /lib/report.js too?

@bcoe
Copy link
Owner Author

bcoe commented Sep 17, 2018

@profnandaa I'd have a preference with just using the new fs.mkdir, but this is something we could patch in Node.js itself; just switch the command used in Node's coverage to set recursive true.

I think we could switch everything that's blocking to async/await, with some fiddling with flow control -- the main advantage I see to doing this is that Mozilla's source-map library is async now:

https://github.com/mozilla/source-map

@profnandaa
Copy link
Contributor

@bcoe -- I'll be on it, hopefully should raise the PR tomorrow (going to bed now).

Thanks for the heads-up about the Mozilla project, checking it out 👍

andela-anandaa pushed a commit to profnandaa/c8 that referenced this issue Sep 18, 2018
- Replace mkdirp with the new fs.mkdir with `recursive` option
- Rewrite all top level IO to async (mkdir and rimraf)

Remaining work: to rewrite lib/*.js to async I/O

Ref: bcoe#9
profnandaa added a commit that referenced this issue Oct 12, 2018
- Replace mkdirp with the new fs.mkdir with `recursive` option
- Rewrite all top level IO to async (mkdir and rimraf)

Remaining work: to rewrite lib/*.js to async I/O

Ref: #9
@bcoe
Copy link
Owner Author

bcoe commented Sep 6, 2019

@profnandaa thank you for this contribution.

@bcoe bcoe closed this as completed Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue hackillinois help wanted Issue is well defined but needs assignment
Projects
None yet
Development

No branches or pull requests

3 participants