nest icon indicating copy to clipboard operation
nest copied to clipboard

Published 20 hours ago • verified publisher icon nestjs

refactor(core): env variable to control slow threshold of token factory

Open H4ad opened this issue 1 year ago • 8 comments

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
  • [x] Tests for the changes have been added (for bug fixes / features)
  • [x] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • [ ] Bugfix
  • [ ] Feature
  • [ ] Code style update (formatting, local variables)
  • [x] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] CI related changes
  • [ ] Other... Please describe:

What is the current behavior?

Today, there's no way to change the threshold we added for slow serialization.

Refs: https://github.com/nestjs/nest/issues/12738#issuecomment-2232426283

What is the new behavior?

Now, we can control the amount of time in MS using NEST_TOKEN_FACTORY_SLOW_THRESHOLD_MS

Does this PR introduce a breaking change?

  • [ ] Yes
  • [x] No

Other information

H4ad avatar Jul 18 '24 22:07 H4ad

Pull Request Test Coverage Report for Build ecf984e0-9894-4582-afce-61a77c6239da

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 92.209%
Totals Coverage Status
Change from base Build 7ea6f01d-9fcc-4d6c-90b4-f217d2306325: 0.001%
Covered Lines: 6746
Relevant Lines: 7316
💛 - Coveralls

coveralls avatar Jul 18 '24 22:07 coveralls

Hi @H4ad , thank you so much for making this PR! Indeed, this warning is driving us crazy today :)

Is anything blocking you from merging it and having it in the next patch version of NestJS? Would be great to have it

kazazor avatar Aug 12 '24 05:08 kazazor

Sorry for nagging here, but can someone from the NestJS team approve/merge this PR by any chance? This would really help a lot of people

kazazor avatar Sep 09 '24 06:09 kazazor

bump

mike-ayres avatar Nov 02 '24 00:11 mike-ayres

/cc @kamilmysliwiec Can we merge this to release in the v11?

H4ad avatar Nov 27 '24 12:11 H4ad

We probably won't need that in v11 as the reference algorithm is going to be used by default. If someone decides to use deep-hash instead, then they will be advised to switch back to reference (instead of bumping the threshold value)

kamilmysliwiec avatar Nov 27 '24 12:11 kamilmysliwiec

Do you think is worthy to include only in v10? Otherwise, I will close this PR

H4ad avatar Nov 27 '24 13:11 H4ad