nest icon indicating copy to clipboard operation
nest copied to clipboard

Revert "Revert "fix: update factory to account for old v4 syntax for cache stores""

Open jmcdo29 opened this issue 3 years ago • 1 comments

This reverts commit 530af92.

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?

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

What is the current behavior?

Reverting a revert, I know, always fun! The flakiness of the tests came from how v5 uses milliseconds whereas v4 uses seconds to the ttl. This caused our tests to not pass because the HTTP calls weren't quite fast enough all of the time.

What is the new behavior?

I updated the timing of the tests to be 100seconds or 100 milliseconds depending on the cache-manager version and that seems to have stabilized the test. The other option we have here would be to automatically multiply the ttl given to the CacheModule.register/Async() by 1000 to convert the ms to s. This would make other user's transition from v4 to v5 smoother, however, if someone were to put in a seconds value already then we've taken it from seconds to a large number of minutes, which would cause the cache to work incorrectly for their use case. It's a tricky one to be sure. Happy to go with either approach though.

Does this PR introduce a breaking change?

  • [ ] Yes
  • [X] No

If anything is a breaking change it's that the ttl is now in milliseconds instead of seconds like it was in v4, but that's a difference of that package and not with out implementation.

Other information

We should update the @nestjs/cli's default webpack to include cache-manager/package.json in the excluded packages

cache-manager@^4 ttl cache-manager@^5 ttl

jmcdo29 avatar Oct 05 '22 17:10 jmcdo29

Pull Request Test Coverage Report for Build da80898b-2ef8-4ef0-85a6-3ca23b7c028c

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.786%

Totals Coverage Status
Change from base Build 6b637f64-9c7a-4d7b-8537-b6a8d098f58e: 0.0%
Covered Lines: 6113
Relevant Lines: 6518

💛 - Coveralls

coveralls avatar Oct 05 '22 17:10 coveralls

lgtm

kamilmysliwiec avatar Oct 24 '22 08:10 kamilmysliwiec