process is forked with a single JSON argument #319
Conversation
| var opts = { | ||
| file: file, | ||
| failFast: this.failFast, | ||
| serial: this.serial, |
There was a problem hiding this comment.
I think we should do this.options = options in the Api constructor so we can just pass the whole options object here instead of explicitly specify each option.
|
Okay, I've removed the direct dependency on "has-flag" (although istanbul still pulls it in), and the child process just uses the parsed JSON options instead of using |
|
See: #319 (comment) |
|
Okay, fork now has the signature: |
| @@ -18,7 +17,7 @@ function Api(files, options) { | |||
|
|
|||
| EventEmitter.call(this); | |||
|
|
|||
| assign(this, options); | |||
There was a problem hiding this comment.
You can't just remove this. The options are used directly on this in the code and needs to be changed to use this.options.x.
There was a problem hiding this comment.
assign(this, options); has been replaced with objectAssign(this, options);
|
I very thoroughly checked for "serial" and "failFast" property access before removing that |
|
@jokeyrhyme It's for example used here: https://github.com/jokeyrhyme/ava/blob/af4e70377a2b0143005c5fad52913821fcbdfbeb/api.js#L150 I would like to remove it though, as I feel it's pretty dirty to have arbitrary options mixed into |
|
It's weird that the tests were still passing when I dropped the |
| @@ -3,6 +3,11 @@ | |||
| var debug = require('debug')('ava'); | |||
|
|
|||
| if (debug.enabled) { | |||
| // Forward the `time-require` `--sorted` flag. | |||
| // Intended for internal optimization tests only. | |||
| if (exports.opts._sorted) { | |||
There was a problem hiding this comment.
Should I completely drop the "time-require" module from package.json dependencies and the related --sorted stuff?
There was a problem hiding this comment.
No, meant that exports.opts._sorted is outdated with the latest PR changes.
|
Very nice work @jokeyrhyme :) Can you squash it down to one commit? |
|
LGTM |
|
Done. :) |
|
@sindresorhus I think your "LGTM" came before my squash :) |
|
LGTM |
|
@sindresorhus This (https://github.com/jokeyrhyme/ava/blob/af4e70377a2b0143005c5fad52913821fcbdfbeb/api.js#L150) is required for serial execution of forks (#282). |
|
@vdemedes in my squashed commit, I include moving that over to |
|
@jokeyrhyme Oh, my bad. Everything is fine then! |
|
LGTM |
process is forked with a single JSON argument
|
@jokeyrhyme Ron, thank you for your valuable contribution to AVA, we really appreciate it! We'd love to see your new PRs soon! |
|
Thank you! :D |
|
🎉 😺 |
This PR attempts to fix #318 .
Initially, the JSON string is converted back into
process.argvvalues in lib/babel.js. I'm happy to go further and actually try to eliminate--fail-fastand--serialaltogether in the child process, but I figured it was worth sharing what I had so far.