Nan if register module with default and use Throttle decorator

[kanopa]

Is there an existing issue for this?

• I have searched the existing issues

Current behavior

When ThrottlerModule.forRoot() is registered with no parameters and @Throttle decorator is set on one of the methods and try to call method where decorator is not set we get Nan in ttlMilliseconds when in storageService trying to add key
const ttlMilliseconds = ttl * 1000;
In memory realisation is we have [Nan]
In https://github.com/kkoomen/nestjs-throttler-storage-redis realisation we have exception

Minimum reproduction code

later

Steps to reproduce

No response

Expected behavior

Ttl must not be Nan, maybe return earlier when ttl is not set or add default value. And need to fix typing from
handleRequest( context: ExecutionContext, limit: number, ttl: number, )

to
handleRequest( context: ExecutionContext, limit?: number, ttl?: number, )

Package version

8.1.3

NestJS version

8.1.3

Node.js version

16.13.1

In which operating systems have you tested?

• macOS
• Windows
• Linux

Other

No response

[jmcdo29]

Please provide a minimum reproduction repository (Git repository/StackBlitz/CodeSandbox project).

[kanopa]

Please provide a minimum reproduction repository (Git repository/StackBlitz/CodeSandbox project).

sure
https://github.com/kanopa/throttler-issue

issue reproduce on default route http://localhost:3000/

i think we need add more stronger typing ttl and limit on handle request, because they could be undefined

If use throttler module without redis error not throwing, but in memory storage saving Nan date

[jmcdo29]

i think we need add more stronger typing ttl and limit on handle request, because they could be undefined

Either that or set defaults that we know will be there. Defaults will be more sensible at the moment as to not need a breaking change.

20 hits per minute is the default of the @Throttle() decorator, should we set the default to that?

If use throttler module without redis error not throwing, but in memory storage saving Nan date

for the redis plugin, please bring that issue up with the package owner. This package is for the main logic and memory storage integration

[kanopa]

i think we need add more stronger typing ttl and limit on handle request, because they could be undefined

Either that or set defaults that we know will be there. Defaults will be more sensible at the moment as to not need a breaking change.

20 hits per minute is the default of the @Throttle() decorator, should we set the default to that?

If use throttler module without redis error not throwing, but in memory storage saving Nan date

for the redis plugin, please bring that issue up with the package owner. This package is for the main logic and memory storage integration

Defaults is good but someone may use logic where he set only limit param and if we set defaults its will be breaking change for him.
I think there are two solutions

1. Add defaults
2. Typing handleRequest to (context: ExecutionContext, limit?: number, ttl?: number) and handle ttl on in memory storage, maybe set infinity instead of NaN or not store values without ttl

Depending on the solution in this package, I will either create a pull request on the redis package or we will solve the problem here :)

[jmcdo29]

I'd like to not have to worry about the handleRequest method, so we'd ensure that there are defaults set by that time. Just want to make sure that what we set are sensible. I know how to handle ensuring they are set and that we don't override though, so that's not an issue

[joZephhh]

Why not having early return as we had previously ?? (see 2c4b2ff), we just need to put it right after the limit and ttl resolution regarding context :

   // Check if specific limits are set at class or route level, otherwise use global options.
    const limit = routeOrClassLimit || this.options.limit;
    const ttl = routeOrClassTtl || this.options.ttl;

     if (typeof limit === 'undefined' || typeof ttl === 'undefined') {
      return true;
    } 

    return this.handleRequest(context, limit, ttl);

https://github.com/nestjs/throttler/blob/ed3c2e354dee3b40f1b8fa09ac6a3c5685ad7b91/src/throttler.guard.ts#LL50C2-L53C52

wdyt ?

[jmcdo29]

I'll need to find when we removed that and why, but it seems reasonable

[mehdibo]

I am also having this issue, setting a default ttl and limit solves the problem.

I am adding this so people like don't waste a lot of time finding out why this error shows up:

[Nest] 562190  - 06/02/2023, 2:17:47 AM   ERROR [ExceptionsHandler] @ntegral/nestjs-sentry:  ERR Error running script (call to f_f7788d43191d9ad66c866e8c63f3c3e361391849): @user_script:5: ERR value is not an integer or out of range
ReplyError: ERR Error running script (call to f_f7788d43191d9ad66c866e8c63f3c3e361391849): @user_script:5: ERR value is not an integer or out of range
    at parseError (/srv/api/node_modules/redis-parser/lib/parser.js:179:12)
    at parseType (/srv/api/node_modules/redis-parser/lib/parser.js:302:14)

[jmcdo29]

Will be fixed in 5.0.0 with #1565

[PatrickOtero]

In my case, nest says that "ttl" does not exists anymore.