Update API #22
Update API #22
Conversation
Use ESM syntax for imports and exports. Ensure compat with previous CJS API.
|
Closing for now, I'll send smaller PRs. |
| close.then((close) => { | ||
| // You can use this handler to perform an action before exiting the | ||
| // foreground child. | ||
| console.log(close.code); |
There was a problem hiding this comment.
How's close a function and object at same time?
There was a problem hiding this comment.
Well, it's like most CJS exports: a function with additional properties (fg() and fg.spawn(), signalExit() and signalExit.signals(), etc.).
It boils down to:
const close = () => { ... };
close.code = code;
close.signal = signal;
return close;| @@ -1,8 +1,8 @@ | |||
| environment: | |||
| matrix: | |||
| - nodejs_version: '5' | |||
| - nodejs_version: '4' | |||
| - nodejs_version: '0.12' | |||
There was a problem hiding this comment.
What's the rationale for dropping support for <6?
There was a problem hiding this comment.
- Align with Node's support policy by targeting oldest LTS.
- Use promise-based API out-of-the box (without having to use
bluebird) - Remove workarounds for old Node versions
- Refactor the internals to use ES syntax (scoped variables, spread args, arrow functions, etc.)
There was a problem hiding this comment.
I see. Looks that just this alone should be a different PR/discussion with the maintainers if they are aligned. That way, it becomes more progressive than big-bang.
| @@ -0,0 +1,13 @@ | |||
| declare module "signal-exit" { | |||
There was a problem hiding this comment.
From our discussion in the channel, I think it's better off dropping off TS from this PR, for now.
There was a problem hiding this comment.
Rewrite this in JS (even not ES6 for now), for backward compatibility.
| @@ -0,0 +1,70 @@ | |||
| import t from "tap"; | |||
There was a problem hiding this comment.
Separate out increasing test-coverage as a different PR, but only have tests for the code you've written for the PR.
This PR is part of the effort to improve Node's support for code coverage (see discussion on
c8).This PR exposes two new functions to support foreground childs:
proxy(parent, child)to start forwarding between two existing processes.spawn(file, args, options)as a replacement for the original functions allowing you to passspawnoptions.The original API is still there. The only breaking change is dropping support for unmaintained Node versions (require Node 6+).
The new
spawnfunction would allow to pass the env NODE_V8_COVERAGE variable directly in c8.There are a few other changes:
cp.spawnfunction (even on Windows), you can pass your own function if you want to.Other comments:
Discussions:
Maybe provide a way to automatically kill the parent process like in the original function?
Migrating from the original function to
fg.spawnshould be relatively easy. For the moment I marked it asdeprecated, I don't think it should be removed immediately (especially since it's known to work fine withnyc). Not sure what you think about it.Closes tap@6.3.0 breaks build 🚨 #13
Closes tap@6.3.2 breaks build 🚨 #14
Closes Update tap to version 7.0.0 🚀 #15
Closes Update tap to version 7.0.1 🚀 #16
Closes Update tap to version 7.1.0 🚀 #17
Closes Update tap to version 7.1.1 🚀 #18
Closes Update tap to version 7.1.2 🚀 #19