-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: configurable maxBuffer
spawn option
#337
feat: configurable maxBuffer
spawn option
#337
Conversation
20c5fb6
to
7d219e5
Compare
timeout
, killSignal
and maxBuffer
timeout
, killSignal
and maxBuffer
index.mjs
Outdated
}) | ||
|
||
if (timeout) { | ||
promise._timeout = setTimeout(() => promise.kill(killSignal), timeout) |
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'm not sure it should go into the library. It's really easy to implement in client code:
let p = $`...`
setTimeout(() => p.kill('SIGINT').catch(_=>_), 1000)
await p
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.
Aha, it's possible to control timeout from a script, but it seems more convenient to hide this logic in API inners just like cp.spawn({timeout: 1000})
does.
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.
Let’s think more of zx’s timeout api. In CP timeout is per execution. And here it will be globally.
test.mjs
Outdated
if (test('spawn timeout')) { | ||
let signal = 0 | ||
$.spawn.timeout = 100 | ||
$.spawn.killSignal = 'SIGKILL' |
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.
Also, I'm not really like this API. timeout & killSignal seems not related here, but they are.
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.
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 mean the difference between:
$.spawn.timeout = 100
$.spawn.killSignal = 'SIGKILL'
and
{ timeout: 100
killSignal: 'SIGKILL'}
test.mjs
Outdated
delete $.spawn.killSignal | ||
assert.equal(signal, 'SIGKILL') | ||
} | ||
|
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.
Probably we can introduce another experimental func (as retry):
await cutoff('SIGKILL', 1000)`...`
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.
What's about $.f({...})
factory?
$.f({maxBuffer: 1024 * 1024 * 1024})`cmd arg1 arg2`
//...
$.nothrow = $.f({noThrow: true})
$.quiet = $.f({quiet: true})
$.retry5 = $.f({retry: 5, delay: 1000})
$.timeout1s = $.f({timeout: 1000})
//...
$.custom = $.f({timeout: 10000, maxBuffer: 1024 * 1024 * 1024, retry: 5, delay: 1000, quiet: true, spawn: customSpawn})
// and finally
global.$ = $.custom
or even factory of factories:
$.custom = $.ff({maxBuffer: $.ff.0, timeout: $.ff.1})
$.custom(1024 * 1024, 1000)`cmd arg1 arg2`
$.retry = $.ff({retry: $.ff.0, delay: $.ff.1})
$.retry(5, 1000)`cmd arg1`
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.
Or maybe introduce $.options
?
Object.assign($.options, {
quiet: true,
retry: 5,
delay: 1000
})
$`cmd args`
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 was thinking about it too for a while.
What about:
let $$ = quiet(nothrow($))
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.
And store options in IIFE contexts? yep, it might work too.
timeout
, killSignal
and maxBuffer
timeout
and maxBuffer
Let’s merge maxBuffers and think about timeouts API in issues? |
np, I'll create another PR for the |
timeout
and maxBuffer
maxBuffer
spawn option
Let's release these changes. @antonmedv, I still suggest to configure cicd to automate this task. Should I create a PR? |
Maintainer: 200 Mb is enough for stdout. No one will use
zx
to test ELK, spark and kuber orchestration.QIWI: Hold my beer )