RateLimiterUnion throws a regular object instead of RateLimiterRes (or somth similar)

[laneme]

Apparently RateLimiterUnion throws an object containing individual RateLimiterRes for each limiters.

So i cannot really easily do a check like this

err instanceof RateLimiterRes

[animir]

@laneme Hey, that's right. It returns an object.
You can read about here on Wiki https://github.com/animir/node-rate-limiter-flexible/wiki/RateLimiterUnion

So i cannot really easily do a check like this

You can check every value in that object

Object.keys(err).forEach((limiterPrefix) => {
  const limiterRes = err[limiterPrefix]
  limiterRes instanceof RateLimiterRes
})

[laneme]

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.

[animir]

@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 keyPrefix.

Could you describe the idea of having its own instance of response?

[laneme]

I think it is quite convenient that we can get specific response by specific limiter keyPrefix

I agree, it would be a dealbreaker if we couldnt do that.

But we can throw UnionRes containing UninonRes.limiters no?

if (error instanceof UninonRes) {
  // do smth with
  error.limiters[keyprefix_A] // RateLimiterRes
}

[animir]

@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?

[laneme]

I'm unaware of all the known errors handled and thrown by this library.
Its possible to organize known errors under different keys, if I understand you of course.

How does it handle different errors in current implementation? Does it still throw the object containing individual Responses?

[animir]

@laneme

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.

[laneme]

Are you saying, because not all limiters failing, it would be more correct to throw an object instead of an error class?

[animir]

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.

[laneme]

Correction, not error class, just class.

I'm saying UnionRes can be just a simple wrapper and contain the very resObj as a property

[animir]

I'm saying UnionRes can be just a simple wrapper and contain the very resObj as a property

Ok, is it better than the object?

[laneme]

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
})

[laneme]

I cannot put a better label tbh, neat would be a good word.

[animir]

Ok, let's imagine, RateLimiterUnion rejects with err.
How would you know you should write this code?

if (error instanceof UninonRes) {
  // do smth with
  error.limiters[keyprefix_A] // RateLimiterRes
}

You would still need to read that in docs, right?

[laneme]

Correct. But i only need to know UninonRes exist. Rest would be typed.

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.
And if you were to pass it to handler functions, i have to provide a typed MyUnionResType to functions

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 null is an object too, which you cannot loop over. And errors are typed unknown
So typeof err == 'object' wouldn't be sufficient everywhere, you need err && typeof err == 'object'.

Wheres with an instance, err instanceof UnionRes would be sufficient.

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.

[animir]

RateLimiterUnion returns an object for resolved and rejected promise - it always returns an object.
You don't need to check if it is object.

If you add UninonRes it will be always returned from the RateLimiterUnion consume method call.
I am not sure how it would help you to check errors.
Am I missing something?

[laneme]

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.
Because all the codebases that have been checking if the error is object, will now silently fail if they dont follow migration steps.

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
}

[animir]

@laneme

union.consume(...).catch(error => /** always object /*)

Yes, exactly. No matter what happens resolved and rejected result is always an object.
More on that, it has a defined type here https://github.com/animir/node-rate-limiter-flexible/blob/master/lib/index.d.ts#L362
It is defined as Record<string, RateLimiterRes>.

I think, this function is a good start, but it isn't enough:

function isRatelimitUnionRes(response) {
    if (
        response &&
        typeof response == 'object' &&
        Object.values(response)[0] instanceof RateLimiterRes
    ) {
        return true
    }
    return false
}

What if the first item is a storage error? Redis may be down?
And the second item in the RateLimiterUnion response may be RateLimiterRes.

The idea of writing a function for checks is interesting.
Could you explain what would you like to check?
Would you like to check if there some error?
In this case, function could be named like isRejectedWithError. In this function you would need to loop through every rejected response in an object and check if it is RateLimiterRes. Basically, that's what I suggested in the beginning.

Object.keys(err).forEach((limiterPrefix) => {
  const limiterRes = err[limiterPrefix]
  limiterRes instanceof RateLimiterRes
})

[laneme]

You see i'm here to improve the usabilty of the union

Yes, exactly.

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 ?
})

it has a defined type here
It is defined as Record<string, RateLimiterRes>.

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.

In this case, function could be named like isRejectedWithError. In this function you would need to loop through every rejected response in an object and check if it is RateLimiterRes. Basically, that's what I suggested in the beginning.

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 RateLimiterUnion could've been different.

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 RateLimiterUnion, and usually should be handled by end users.

Maybe what you do is provide a recipe block in the docs, Recipe: How to combine multiple limiters.

When the project has enough bandwidth to support a proper Union, that should have a different architecture than a combine utility hidden behind a class mask. Throwing potential different types of errors in a list imo cause bad experience. I can only give limited ideas but heres one anyway:

1. Instead of accepting other limiters (which can have random storage api, different key prefix)

• Make a complete RateLimiterUnion that accepts a list of window preferences.
• It should allow a single storage interface, single keyprefix
• Should function just like an individual limiter RateLimiterRedis

2. Or just have individual limiters like RateLimiterRedis accept list of window preferences.

On the rare case, when people want to combine multiple limiters with different storage, they can always look at the recipe.

[animir]

Thank you for the ideas.

No, this is not how people catch in the wild. See this two examples

Those two examples above are usually enough.
Any user would like to consume from two or more limiters and have a unified response.
Most of us don't need to know what kind of errors there are in the object.
You configure limiters, create union and consume from it. If points consumed - great! - do something. If points are not consumed - great - do something else. That perfectly fits the resolve-reject model with two outcomes.

Could you explain your case again? What your code should do when you know it is RateLimiterRes response or error?