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

feat: configurable maxBuffer spawn option #337

Merged
merged 3 commits into from Mar 13, 2022

Conversation

antongolub
Copy link
Contributor

@antongolub antongolub commented Mar 13, 2022

Maintainer: 200 Mb is enough for stdout. No one will use zx to test ELK, spark and kuber orchestration.
QIWI: Hold my beer )

  • Tests pass
  • Appropriate changes to README are included in PR

@antongolub antongolub changed the title feat: add default spawn properties override: timeout, killSignal and maxBuffer feat: override default spawn properties: timeout, killSignal and maxBuffer Mar 13, 2022
index.mjs Outdated
})

if (timeout) {
promise._timeout = setTimeout(() => promise.kill(killSignal), timeout)
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

index.mjs Outdated Show resolved Hide resolved
test.mjs Outdated
if (test('spawn timeout')) {
let signal = 0
$.spawn.timeout = 100
$.spawn.killSignal = 'SIGKILL'
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

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')
}

Copy link
Collaborator

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)`...`

Copy link
Contributor Author

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`

Copy link
Contributor Author

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`

Copy link
Collaborator

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($))

Copy link
Contributor Author

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.

@antongolub antongolub changed the title feat: override default spawn properties: timeout, killSignal and maxBuffer feat: override default spawn properties: timeout and maxBuffer Mar 13, 2022
@antonmedv
Copy link
Collaborator

Let’s merge maxBuffers and think about timeouts API in issues?

@antongolub
Copy link
Contributor Author

antongolub commented Mar 13, 2022

np, I'll create another PR for the timeout option.

@antongolub antongolub changed the title feat: override default spawn properties: timeout and maxBuffer feat: configurable maxBuffer spawn option Mar 13, 2022
@antonmedv antonmedv merged commit 15d500d into google:main Mar 13, 2022
@antongolub
Copy link
Contributor Author

antongolub commented Mar 14, 2022

Let's release these changes.

@antonmedv, I still suggest to configure cicd to automate this task. Should I create a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants