Skip to content

Improve setTimeout ergonomics #88

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

Merged
merged 1 commit into from
Apr 23, 2025

Conversation

julien-deoux
Copy link
Contributor

This draft proposes a very conservative change to improve timeout-related bindings, but is mostly meant to help me align with the project's strategy:

  • Do we want to remove the possibility of calling setTimeout with a string altogether, as suggested in Improve generated overloads #27?
  • Should we introduce a timeoutId phantom type like in the Core library? If so, should we make use of the existing one or should we declare our own in a place like DOMAPI.res (which makes my editor crash btw)?
  • Do we want to allow float timeouts like Core? This Stackoverflow thread suggests that setTimeout's behaviour with floats isn't specified and thus could be inconsistent across runtimes.

@nojaf
Copy link
Collaborator

nojaf commented Apr 21, 2025

Hi there, thanks for raising another PR, much appreciated!

setTimeout is an interesting one, it makes you wonder whether having it in Core is right to begin with. As it seems to be a runtime specific thing, that virtually any giving runtime will have.

Do we want to remove the possibility of calling setTimeout with a string altogether

Yes, that sounds very reasonable. I don't think we cater to an audience that would ever consider that. Let's remove it.

Should we introduce a timeoutId phantom type like in the Core library?

We could, placing it in DOMAPI would be the in line with the rest.

DOMAPI.res (which makes my editor crash btw)

That is interesting, do you have a stack trace or anything there?
I understand the hint to place it in another file, though I'm not sure here.
Could be a Prelude.res thing maybe.

Do we want to allow float timeouts like Core?

I'm not sure that is worth it. I would add that if a lot of people have this use-case.
If a single person has it, a Obj.magic can also transform that float into an int.

@julien-deoux julien-deoux marked this pull request as ready for review April 22, 2025 18:28
@julien-deoux
Copy link
Contributor Author

I understand the hint to place it in another file, though I'm not sure here.

That wasn't a hint, I fully agree that DOMAPI.res is only the place that makes sense based on the structure of the project. ^^
Incidentally, I realised that my editor doesn't actually crash on this file, but is just really slow to respond (I heard that neovim is notoriously bad at handling big files).

I updated the PR based on your suggestions.

Copy link
Collaborator

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Thank you!

@nojaf nojaf merged commit 36cb2d1 into rescript-lang:main Apr 23, 2025
2 checks passed
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.

2 participants