-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
RateLimiterUnion throws a regular object instead of RateLimiterRes (or somth similar) #279
Comments
@laneme Hey, that's right. It returns an object.
You can check every value in that object Object.keys(err).forEach((limiterPrefix) => {
const limiterRes = err[limiterPrefix]
limiterRes instanceof RateLimiterRes
}) |
Ik but it adds a touch of inconvenience, tad bit more for the hovering user who had to pull up the docs again to be sure. Unless there is a reason for it, Can we give it its own instance? Maybe as part of the next breaking release. |
@laneme When a RateLimiterUnion throws an error a developer would like to know what happened with one, two, or more limiters combined as a union. There should be a complex response. I think it is quite convenient that we can get specific response by specific limiter Could you describe the idea of having its own instance of response? |
I agree, it would be a dealbreaker if we couldnt do that. But we can throw if (error instanceof UninonRes) {
// do smth with
error.limiters[keyprefix_A] // RateLimiterRes
} |
@laneme That's almost good. There is one complexity you didn't consider. One limiter can throw some fatal error, and another can throw RateLimiterRes. It can be a mix of errors. What should RateLimiterUnion throw in this case? |
I'm unaware of all the known errors handled and thrown by this library. How does it handle different errors in current implementation? Does it still throw the object containing individual Responses? |
Yes, that's why the object with keys. One response can be successful RateLimiterRes, another can be an error of any type. |
Are you saying, because not all limiters failing, it would be more correct to throw an object instead of an error class? |
I am saying there could be two failing limiters. Developer should be able to access those errors and do something about both errors. |
Correction, not error class, just class. I'm saying |
Ok, is it better than the object? |
Yes exactly. This code is not just for working with the union error but also to check if the error is indeed the union error, while the check could've been done by a instance check Object.keys(err).forEach((limiterPrefix) => {
const limiterRes = err[limiterPrefix]
limiterRes instanceof RateLimiterRes
}) |
I cannot put a better label tbh, neat would be a good word. |
Ok, let's imagine, RateLimiterUnion rejects with err. if (error instanceof UninonRes) {
// do smth with
error.limiters[keyprefix_A] // RateLimiterRes
} You would still need to read that in docs, right? |
Correct. But i only need to know And i did check docs the first time, but i still wasn't sure because i rarely see raw objects thrown. So instead of relying on docs & even my console log, i had to look at the source to be sure. My confusion isnt the only reason i came for a discussion. see the check blocks if (typeof error == 'object' && Object.values(error)[0] instanceof RateLimiterRes) // error itself not typed yet
// vs
if (error instanceof UnionRes) // error typed i have to manually type the error to get typing inside the if block. Full picture (with your code) let isUnionError = false
// forEach needs to be async loop, cause error handling components can be async
typeof err == 'object' &&
Object.keys(err).forEach(limiterPrefix => {
// err[limiterPrefix] isn't typed
const limiterRes = err[limiterPrefix]
if (limiterRes instanceof RateLimiterRes) {
// now i get typing on limiterRes
isUnionError = true
}
})
// dont go down, handled above (or handle union errors at last)
if (isUnionError) {
return
} Or shorter if (
typeof err == 'object' &&
Object.values(err)[0] instanceof RateLimiterRes
) {
// assert, cause err isnt fully typed
handleLimiterUnionError(err as MyUnionResType)
} Also to note that, typeof Wheres with an instance, Lmk if i'm wrong with something. The limiter is used differently in different components of an app, meaning users have to have a separate checker/typeguard for unionRes to have the desired experience. |
RateLimiterUnion returns an object for resolved and rejected promise - it always returns an object. If you add |
If i understand you, you're saying everyone uses the library like this? union.consume(...).catch(error => /** always object /*) Even in that world, you should provide some typing for end user. Providing interfaces for users has nothing to do with library always working as expected. Anyway, doing the change i proposed even in a breaking change will be hard and can be be catastrophic. Maybe export this typeguard for us? Its non breaking too // lib/index.d.ts
export type LimiterUnionRes = Record<string, RateLimiterRes>
export function isRatelimitUnionRes(res: unknown): res is LimiterUnionRes // lib/RateLimiterUnion.js
function isRatelimitUnionRes(response) {
if (
response &&
typeof response == 'object' &&
Object.values(response)[0] instanceof RateLimiterRes
) {
return true
}
return false
}
module.exports = { isRatelimitUnionRes, RateLimiterUnion } Usage if (isRatelimitUnionRes(error)) {
// error[keyprefix_x]. is typed
} |
Yes, exactly. No matter what happens resolved and rejected result is always an object. I think, this function is a good start, but it isn't enough:
What if the first item is a storage error? Redis may be down? The idea of writing a function for checks is interesting.
|
You see i'm here to improve the usabilty of the union
No, this is not how people catch in the wild. See this two examples try {
// they can parse ip, userid or whatever, ERRORED
await union.consume(...)
// they can continue doing more stuff, ERRORED
} catch (error) {
// error is what ?
} union.consume(...).then(
// code inside then block can throw other errors
).catch(error => {
// error is what again ?
})
Thats a return type. Only works when the promise resolves, promise rejections aren't typed. Even if it did, its good practice to export them the record type.
The loop you provided is incomplete too, shows type errors and importantly is not exported from the lib. Exporting a util/typeguard is what i'm here for. You know better the possible types of errors. Maybe needs someth like this: function isRatelimitUnionRes(response) {
if (
response &&
typeof response == 'object' &&
- Object.values(response)[0] instanceof RateLimiterRes
+ Object.values(response).find(res => isKnownErrorByThisLibrary(res))
) {
return true
}
return false
} Lastly, on a separate note, i think the api design of the Looking at the source, i see all it does is loops over the ratelimiters and manages an array of the limiter errors/responses. This is the type of implementations that doesn't need (or shouldn't have) it's own interface Maybe what you do is provide a recipe block in the docs, When the project has enough bandwidth to support a proper
On the rare case, when people want to combine multiple limiters with different storage, they can always look at the recipe. |
Thank you for the ideas.
Those two examples above are usually enough. Could you explain your case again? What your code should do when you know it is RateLimiterRes response or error? |
Apparently
RateLimiterUnion
throws an object containing individualRateLimiterRes
for each limiters.So i cannot really easily do a check like this
The text was updated successfully, but these errors were encountered: