abort all processes when one fails #53
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,57 +1,125 @@ | ||
| import spawn from 'spawn-command-with-kill' | ||
| import Promise from 'bluebird' | ||
| import colors from 'colors/safe' | ||
| import {isString, clone} from 'lodash' | ||
| import {sync as findUpSync} from 'find-up' | ||
| import managePath from 'manage-path' | ||
| import arrify from 'arrify' | ||
| import getScriptToRun from './get-script-to-run' | ||
| import getScriptsFromConfig from './get-scripts-from-config' | ||
| import getLogger from './get-logger' | ||
|
|
||
| const NON_ERROR = 0 | ||
|
|
||
| export default runPackageScripts | ||
|
|
||
| function runPackageScripts({scriptConfig, scripts, args, options = {}}) { | ||
| if (scripts.length === 0) { | ||
| scripts = ['default'] | ||
| } | ||
| const scriptNames = arrify(scripts) | ||
| if (options.parallel) { | ||
|
Collaborator
Author
There was a problem hiding this comment. I feel like the code below could be greatly simplified with RxJS. But I have no idea how to use Observables. Someone please help me! There was a problem hiding this comment. I don't really see an opportunity for observables. Might be missing something though. Why do you think observables make sense here?
Collaborator
Author
There was a problem hiding this comment. I've never used Observables, but I thought that they're supposed to really help with complex async stuff. 🤷 There was a problem hiding this comment. I think it can be done a little bit cleaner though. I'll do a PR later today or this weekend. There was a problem hiding this comment. Maybe
Collaborator
Author
There was a problem hiding this comment. I don't know what I'm looking for.. I just think this code could be cleaned up a bit... There was a problem hiding this comment. When running in parallel, you can just use const result = promises.reduce((res, task) => {
return res.then(() => task());
}, Promise.resolve());
result.then(...); |
||
| return runParallel() | ||
| } else { | ||
| return runSeries() | ||
| } | ||
|
|
||
| function runSeries() { | ||
| return scriptNames.reduce((res, scriptName) => { | ||
| return res.then(() => ( | ||
| runPackageScript({scriptConfig, options, scriptName, args}) | ||
| )) | ||
| }, Promise.resolve()) | ||
| } | ||
|
|
||
| function runParallel() { | ||
| const results = scriptNames.map(script => ({script, code: undefined})) | ||
| let aborted = false | ||
|
|
||
| const promises = scriptNames.map(scriptName => { | ||
| return runPackageScript({scriptConfig, options, scriptName, args}) | ||
| }) | ||
|
|
||
| const allPromise = Promise.all(promises.map((promise, index) => { | ||
| return promise.then(code => { | ||
| if (!aborted) { | ||
| results[index].code = code | ||
| } | ||
| }) | ||
| })).then(() => results) | ||
|
|
||
| allPromise.catch(() => { | ||
| /* istanbul ignore if */ | ||
| if (aborted) { | ||
| // this is very unlikely to happen | ||
| } else { | ||
| abortAll() | ||
| } | ||
| }) | ||
|
|
||
| return allPromise | ||
|
|
||
| function abortAll() { | ||
| aborted = true | ||
| promises.forEach(p => p.abort()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| function runPackageScript({scriptConfig, options, scriptName, args}) { | ||
| const scripts = getScriptsFromConfig(scriptConfig, scriptName) | ||
| const script = getScriptToRun(scripts, scriptName) | ||
| if (!isString(script)) { | ||
| return Promise.reject({ | ||
| message: colors.red( | ||
| `Scripts must resolve to strings. There is no script that can be resolved from "${scriptName}"` | ||
| ), | ||
| ref: 'missing-script', | ||
| }) | ||
| } | ||
| const command = [script, args].join(' ').trim() | ||
| const log = getLogger(getLogLevel(options)) | ||
| log.info(colors.gray('p-s executing: ') + colors.green(command)) | ||
| let child | ||
| const promise = new Promise((resolve, reject) => { | ||
| child = spawn(command, {stdio: 'inherit', env: getEnv()}) | ||
|
|
||
| child.on('error', error => { | ||
| child = null | ||
| reject({ | ||
| message: colors.red( | ||
| `The script called "${scriptName}" which runs "${command}" emitted an error` | ||
| ), | ||
| ref: 'emitted-an-error', | ||
| error, | ||
| }) | ||
| }) | ||
|
|
||
| child.on('close', code => { | ||
| child = null | ||
| if (code === NON_ERROR) { | ||
| resolve(code) | ||
| } else { | ||
| reject({ | ||
| message: colors.red( | ||
| `The script called "${scriptName}" which runs "${command}" failed with exit code ${code}` | ||
| ), | ||
| ref: 'failed-with-exit-code', | ||
| code, | ||
| }) | ||
| } | ||
| }) | ||
| }) | ||
|
|
||
| promise.abort = function abort() { | ||
| if (child !== null) { | ||
| child.kill() | ||
| child = null | ||
| } | ||
| } | ||
|
|
||
| return promise | ||
| } | ||
|
|
||
| function getLogLevel({silent, logLevel}) { | ||
|
|
||
Oops, something went wrong.
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.
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.
Full lodash is quite huge, why not install them separately?
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 get you, but honestly I'm using about a dozen methods and I'd rather avoid installing each separately/managing them. It's 1.4MB (uncompressed) and the likelihood that someone's already depending on lodash in their project (even if it's transitively) is quite high.
Thanks for the suggestion.