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

string support for timeouts #452

Closed
devhawk opened this issue May 21, 2024 · 7 comments
Closed

string support for timeouts #452

devhawk opened this issue May 21, 2024 · 7 comments

Comments

@devhawk
Copy link
Collaborator

devhawk commented May 21, 2024

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
Copy link
Contributor

demetris-manikas commented May 21, 2024

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
Copy link
Contributor

@devhawk Any updates on this?

@devhawk
Copy link
Collaborator Author

devhawk commented May 22, 2024

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

@qianl15
Copy link
Member

qianl15 commented May 22, 2024

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
Copy link
Contributor

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

@demetris-manikas
Copy link
Contributor

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
Copy link
Member

qianl15 commented May 29, 2024

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

@kraftp kraftp closed this as not planned Won't fix, can't repro, duplicate, stale Jun 5, 2024
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

No branches or pull requests

4 participants