search

dbos-inc / dbos-transact

The TypeScript framework for backends that scale
https://docs.dbos.dev
MIT License
291 stars 20 forks source link

string support for timeouts #452

Closed devhawk closed 1 month ago

devhawk commented 1 month ago

We should consider adding support for strings like "2 days", "30s" by using the ms package.

Originally part of #449, but split off into a separate issue because #449 is a breaking change and this change is not

demetris-manikas commented 1 month ago

Shoot. I cannot import from ms no matter what. Whatever the import is I get the following error

this module can only be referenced with ECMAScript imports/exports by turning on the 'allowSyntheticDefaultImports' flag and referencing its default export

Which is on due to

"esModuleInterop": true, /* Emit additional JavaScript to ease support for importing CommonJS modules. This enables 'allowSyntheticDefaultImports' for type compatibility. */ in tsconfig.

Also the ms package looks like it has been abandoned or at least not actively maintained. Last stable version was released back in 2021.

I also tried to import the canary version, which is in canary state since 2021 too, to no avail.

any ideas?

demetris-manikas commented 1 month ago

@devhawk Any updates on this?

devhawk commented 1 month ago

This was originally @qianl15 suggestion. I don't know anything about the ms package myself

qianl15 commented 1 month ago

I tested locally and it seems that we need to also add "allowSyntheticDefaultImports": true to the tsconfig.json Then use import ms from 'ms'; should work.

demetris-manikas commented 1 month ago

I did that but it kept complaining. I will retry and get back to you

demetris-manikas commented 1 month ago

I have created a PR ( #474) on this but I have to say I don't like the implementation at all but I did not manage any better. I did not add the option in the sleepms as it's name is too specific (imagine calling sleepms("1m30s")).

The things I don't like ...

1) I would avoid introducing a so old package in the codebase (specially one that only provides convenience and not missing functionality) but that is on you. Also the user can import the package on it's own and call sleepms(ms('1m30s'))

2) To have a typed function I did the 'hack' of copying the types from the canary version but this can go away if you insist on adopting the ms package and don't approve of the 'hack'.

3) I don't like the interface sleep(durationSec: number | StringValue) and would probably hate sleep(durationSec: number | string)
The former is hardly usable while the latter can be very misleading. Myself I would try to call sleep('1') believing it will sleep for a second as the name of the parameter implies.

All in all I suggest that the whole idea be dropped for now.

qianl15 commented 1 month ago

Hi @demetris-manikas thanks for exploring this idea! I see the problems here and I agree that we should table this for now.