kit
kit copied to clipboard
sveltejs
review which APIs are async vs not
Describe the problem
We have a bunch of functions which are unnecessarily async, but we can't change them because they're public. They're potentially harder to call since they're async as they need to then be called from a function which is itself async.
We also have a lot of public APIs that use MaybePromise, which seems like a poor practice. It would be better to always return a Promise in these instances. E.g. if an error occurs while calling you would only need to handle promise rejection rather than both that and wrapping in a try/catch
preloadCode returns a Promise, but I wonder if it needs to or should. We invoke it internally in numerous places and always ignore rejected promises letting them go the console like any network error would. If users care about these errors they already need to listen for unhandledrejection. I don't know if users would ever want to do something different only for explicit programmatic invocations of preloading as compared to hovering a link
Describe the proposed solution
Review places we have eslint-disable-next-line @typescript-eslint/require-await
Alternatives considered
Leave them async in case we want to make more code async in the future. But if we're doing that if feels like we should make every public API async. Right now there doesn't seem to be much rhyme or reason to it
Importance
nice to have
Additional Information
No response
Do you mean review the places we have that on another branch? We don't appear have any instances of that on main.
Rich added a couple of TODOs for this in https://github.com/sveltejs/kit/pull/12297
Actually, we need to be careful about those TODOs. If you turn on the no-misused-promises and no-floating-promises rules from typsescript-eslint you can see that we're missing some await statements and perhaps those should be async and we should just add this missing await