Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
typings: add JSDoc typings for timers#38834
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Added JSDoc typings for the `timers` lib module.
VoltrexKeyva commented May 28, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
On a note, the |
apapirovski left a comment
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
apapirovski commented Jun 11, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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()`.
VoltrexKeyva commented Jun 11, 2021
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. |
apapirovski commented Jun 11, 2021
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. |
apapirovski commented Jun 11, 2021
@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} */ |
VoltrexKeyva commented Jun 11, 2021
@apapirovski sadly that doesn't work, stays the same type as |
apapirovski commented Jun 11, 2021
Interesting. The documentation calls this out as the solution to using |
VoltrexKeyva commented Jun 11, 2021
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 <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
targos commented Jul 11, 2021
Landed in c4096a3 |
Added JSDoc typings for the `timers` lib module. PR-URL: #38834 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Added JSDoc typings for the `timers` lib module. PR-URL: #38834 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Added JSDoc typings for the
timerslib module.