-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
typings: add JSDoc typings for timers #38834
Conversation
Added JSDoc typings for the `timers` lib module.
On a note, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of small clarifications but the rest looks good.
Spread operator was slower than what we have, last time I tried it, but you could always make that change locally and see how the benchmarks perform with it. Other than that, I'm not super familiar with JSDoc intricacies but I assume that replacing arg1, arg2, arg3 with |
Fixed parameter type of `clearTimeout()` and `clearInterval()`.
yea, JSDoc does not enforce typings if the code does not match; so the only of doing this is replacing those 3 parameters at the end with a rest parameter but that may cause a perf regression, so I think we should just let it either be this way as it is, or maybe change the parameter to a rest one and benchmarking them, overall the intension of the function is the most important. |
Would definitely encourage trying this locally. I don't know if it's been tried in over a year at this point so the performance could've easily gotten on par. If you prefer not to run them locally, can always open a PR and run the benchmarks in CI. |
@VoltrexMaster so looking at JSDoc documentation, it seems like we could actually get away with https://jsdoc.app/tags-param.html#multiple-types-and-repeatable-parameters where you specify arg3 as being repeated. It's a bit ugly but at least gets information across? So it seems like you could do /**
* Schedules the immediate execution of `callback`
* after I/O events' callbacks.
* @param {Function} callback
* @param {any} [arg1]
* @param {any} [arg2]
* @param {...*} [arg3]
* @returns {Immediate}
*/ |
@apapirovski sadly that doesn't work, stays the same type as |
Interesting. The documentation calls this out as the solution to using |
Imma just let it stay like this, I may try to benchmark the functions after applying rest parameters in the future and open a PR to make the changes if a perf regression doesn't happen. |
Added JSDoc typings for the `timers` lib module. PR-URL: #38834 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Landed in c4096a3 |
Added JSDoc typings for the `timers` lib module. PR-URL: #38834 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Added JSDoc typings for the `timers` lib module. PR-URL: #38834 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Added JSDoc typings for the
timers
lib module.