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

Ergonomics of timeoutSeconds is rough #130

Closed
sam-goodwin opened this issue Dec 21, 2022 · 4 comments · Fixed by #221
Closed

Ergonomics of timeoutSeconds is rough #130

sam-goodwin opened this issue Dec 21, 2022 · 4 comments · Fixed by #221

Comments

@sam-goodwin
Copy link
Owner

We only support a granularity of seconds, which is why our timeouts use timeoutSeconds, but libraries like ms return milliseconds. This leads to an unfortunate usage such as:

ack.expect({
  timeoutSeconds: 60 * 60
});

ack.expect({
  timeoutSeconds: ms("1 hour") / 1000
});

Should we allow for a timeout property that accepts a string compatible with ms? Temporal also does this.

ack.expect({
  timeout: "1 hour"
});

ack.expect("1 hour");
@thantos
Copy link
Contributor

thantos commented Dec 21, 2022

I didn't like ms because of all of the debate and ambiguity around months and weeks (vercel/ms#57) and the complexity it caused for temporal.

https://legacy-documentation-sdks.temporal.io/typescript/workflows#sleep

// NOT VALID
await sleep('1 month'); // ms package doesnt support "months" https://github.com/vercel/ms/issues/57
// use date-fns and sleepUntil instead, see below

While the english form it quite pretty, it:

  1. supports milliseconds while we don't. Millisecond values will caused undefined behavior while seconds are fairly accurate.
  2. cannot be computed without first formatting to a string
  3. creates ambiguity like how to compute a month or a week that would not be present when just providing seconds.

What other options are there?

  1. update to accept milliseconds instead of seconds with warnings everywhere that milliseconds are not supported. timestampMilliseconds (must be a multiple of 1000)
  2. an object form { seconds: number; minutes: number: ... }
  3. iso duration 1D10m
  4. Use the new "temporal" ecma spec (which I thought was too complex for our use case)

@sam-goodwin
Copy link
Owner Author

Are those really big enough problems to not support the ms format? You can paper over their rough edges and throw an error when it's milliseconds. Still might be better than forcing everyone to do calculations. It wouldn't have to be the only way - if people want to compute a time they can still use the seconds api.

Is there a way in which we can support all of these use cases? Temporal api looks good - standardizing around iso format seems right.

I guess for now we can leave it as it is - let people compute seconds and wait for it to be a problem and then discuss with users instead of extra breadth now.

@thantos
Copy link
Contributor

thantos commented Dec 27, 2022

timeout: duration(20, "seconds")?

@thantos
Copy link
Contributor

thantos commented Jan 11, 2023

#158 (comment)

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 a pull request may close this issue.

2 participants