Upcoming releases: v6.10 & v7

[nfriedly]

I have a few things in mind for the v7 release, some of which would benefit from warning users of ahead of time:

v6.10.0 - done and shipped!

• Warn for use of onLimitReached
  • PR: validations: add onLimitReached deprecation warning #373
• Warn for use of max=0
  • PR: validations: add max: 0 change warning #370
  • Previous discussion: Changing behavior of max=0 #369
  • consider modifying the validator to let this check run indefinitely, or at least for more than the first request
• Add support for draft-7 standard headers
  • PR: feat: support combined RateLimit header from ietf draft 7 #376
• Warn for draft_polli_ratelimit_headers
  • PR: validations: add draft_polli_ratelimit_headers option deprecation warning #377
• readme: use named import for both cjs and esm
  • This should help to avoid Not callable expression in 6.7.1 #360
  • Commit: 9ba2ed2
• Add a new validation check that hits is a positive number (rateLimit not being update #353)
  • Also, fix the root cause in the Memcached store
    • Not really a bug, they just tried something undocumented and it failed in an unexpected way. However, it works when following the documentation. A validation check is probably sufficient.
  • PR: feat: validate totalHits value returned from store #379

v7.0.0 - done and shipped!

• change max=0 behavior to just call handler on every request
  • update warning to indicate that the change has now happened
  • PR: fix!: max: 0 should rate limits all requests #380
• Drop support for draft_polli_ratelimit_headers option
  • update the warning
• drop support for onLimitReached
  • update the validation error
  • PR: chore: drop deprecated options #382
• upgrade dts-bundle-generator and typescript
• Bump min node.js version to node 16.
  • Related: bump the typescript target to ES2022 per https://stackoverflow.com/questions/67371787/what-typescript-configuration-produces-output-closest-to-node-js-16-capabilities/67371788#67371788
  • PR: build: drop node 14 #383
• Updated MemoryStore
  • PR: feat!: improve MemoryStore #378
• Deprecate max, replace it with limit.
  • Continue to support both without a warning for now.
  • PR: feat: rename max to limit while still supporting max #381
• Consider making validations more flexible - allow some to run on more than one request, allow them to be individually disabled
  • PR for individual disabling: Allow specific validations to be enabled or disabled #392
• Rename req.rateLimit.current to req.rateLimit.used
  • PR: fix: rename current to used, while still supporting current #391

Not going to change after all

Headers

Given that the standard headers draft is still undergoing significant changes, I think we should skip most of the warnings around headers (except the draft_polli_ratelimit_headers option), and definitely not change the defaults.

v6.10:

• Warn for v5 and earlier headers option
• Warn for unset headers options

v7

• change headers defaults to standardHeaders: true, legacyHeaders: false
• update the warning for now, probably remove it eventually
• Maybe add a warning for standardHeaders: true?
• Drop support for v5 headers options

Other changes omitted from v7:

• maybe suppress max=0 warning if a custom handler is set (this will be the case for express-slow-down and could conceivably be helpful elsewhere)
  - the ability to disable specific validations mostly obviates the need for this
• Maybe: switch esbuild to tsc to use decorators?
  • not needed, I worked out a solution to disable specific validators without needing this.

[gamemaker1]

Should we also drop node 14? It's been EOL for almost 2 years, and active security support ended 3 months ago.

change headers defaults to standardHeaders: true, legacyHeaders: false

We should also remove the draft_polli option.

switch esbuild to tsc to use decorators?

We could try out tsc for the decorators, if it makes writing validations easier.

[nfriedly]

Should we also drop node 14? It's been EOL for almost 2 years, and active security support ended 3 months ago.

Yeah, seems like a good idea, especially if it's making testing more difficult.

We should also remove the draft_polli option.

Yes, and we should add a warning for that now too.

switch esbuild to tsc to use decorators?

We could try out tsc for the decorators, if it makes writing validations easier

Yeah, I'm not 100% on this one, but I might play with it some.

I'll update the top post with the other couple of things.

[gamemaker1]

I was wondering if we should drop the default export too - and only have a named export. It'll allow us to get rid of the ugly footer we tape on to the end of the cjs files.

[nfriedly]

No, I think that'd be too big of a breaking change. The changes I want to make shouldn't affect anyone who's been following our guidelines since the V6 release, but removing the default export would break basically everyone who followed our instructions up to this point.

I suppose we could deprecate it in V7 for removal in an eventual V8 release, but even that feels like too much. I think we're stuck with the ugly footer for now.

Honestly, I'd be more inclined to deprecate the cjs release entirely, and switch to pure esm. But I'm not sure if it's too soon for that.

I kind of wish we had some analytics. But, even making that opt-in probably wouldn't fly well with some folks. And we'd only get usage data for releases after we added it, anyways.

[gamemaker1]

I think we should deprecate it in v8, to set up the path for an eventual ESM-only release. Although, as you said, that might take a lot more time - maybe even a couple of years.

I kind of wish we had some analytics. But, even making that opt-in probably wouldn't fly well with some folks. And we'd only get usage data for releases after we added it, anyways.

We could try to parse through the github repos that use our package using a script, that might give us a hint.

[nfriedly]

Hmm.. although if we switch to pure esm, other esm projects could still use a default import, it's only cjs projects that would need to change. Still feels like too big of a breaking change, though.

I do like the idea of parsing through all the projects that depend on erl!

[nfriedly]

Hum, it looks like the headers standardization went through a major change: https://author-tools.ietf.org/iddiff?url1=draft-ietf-httpapi-ratelimit-headers-06&url2=draft-ietf-httpapi-ratelimit-headers-07&difftype=--html

Instead of separate headers for limit, remaining, and reset, they're now fields in a single header.

I think we'll want to support this, but now I'm not sure about making it the default since it's still changing. I also think we want some way to request the behavior from the earlier draft. I don't like true pointing to an older version, but I also don't like changing the behavior under everyone's feet. Maybe we'll require it be a value like draft-6 or draft-7 and warn, but use draft-6 if it's set to true?

[nfriedly]

Now that the rate limit header is getting more complex, maybe we should write a parser? Probably as a separate package though.

[gamemaker1]

No, I think we should keep the behavior of true the same for v7, but just warn about it. I don't think we should change its behavior until the draft is fully standardized. Then we'll do another Semver major release (v8 or whatever) where we make true activate whatever the final standardized behavior is.

Alright!

[gamemaker1]

Now that the rate limit header is getting more complex, maybe we should write a parser? Probably as a separate package though.

Sure, that sounds like a good idea.

[nfriedly]

I did some initial work on a headers parser. It works for the couple of happy-cases tests I wrote, but it still needs some work before it's ready to ship: https://github.com/express-rate-limit/ratelimit-header-parser

[gamemaker1]

Oooh nice! I'll help with the todos.

[nfriedly]

I just had a thought: for deprecation notices, we've been using WRN rather than ERR in the codes. That makes sense right now, but are we going to want to make them errors rather than warnings in 7.x? Perhaps the codes should just be ERR from the start then?

[gamemaker1]

I think deprecation notices should be only warnings, not errors.

Even when we remove the options in v7, the notices are still warning users that expected behaviour might not be seen, because they used the wrong option.

[nfriedly]

One more change I think we should make is to rename the max option to limit for consistency. But I think we'll have to keep max around as a deprecated option indefinitely, so I don't think it needs a validation check. Not technically a breaking change, but probably a good fit for 7.x anyways.

[gamemaker1]

Alright!

[nfriedly]

Last PR for 6.10 is up: #379

[gamemaker1]

Merged! Waiting on #376, let's do a release once that is merged?

[gamemaker1]

Kind of a note to myself because I thought of this twice today and came to the same conclusion both times:

Can't use pkgroll because it outputs CJS with the following exports -

exports.MemoryStore = MemoryStore;
exports["default"] = rateLimit;
exports.rateLimit = rateLimit;

So anyone using:

const rateLimit = require('express-rate-limit')

will hit an error that says rateLimit is not a function. Instead, they would need to use:

const rateLimit = require('express-rate-limit').default

Ppl using typescript with esModuleInterop: true or allowSyntheticDefaultImports: true won't need to make this change, since typescript does it for them.

However, I feel it would be quite presumptous if we forced JS users to change their imports just so we could make bundling the library easier for us. Instead, we could shift to pkgroll when we switch to only named exports, which is the recommended method of exporting things in a dual-cjs-esm package.

---

The benefit of switching to pkgroll is that all three of our build commands - build:cjs,esm,types - get replaced with a single line: pkgroll --target es2022 --src source/, and we don't need to worry about issues like #361 again.

[nfriedly]

Hey, this is kind of unrelated, but did something change with our jest setup in the past few weeks? I used to be able to run individual tests from within vs code, and now that seems broken.

[gamemaker1]

I did move the jest configuration to a separate file - config/jest.json, maybe VSCode hasn't detected that automatically?

[nfriedly]

Yeah, that might be it. Was it in package.json before?

[gamemaker1]

Yea

[gamemaker1]

I was actually planning to move all package.json config into separate config files, for jest and xo and lint-staged.

[nfriedly]

#387 fixes jest testing in vs code. It's still in a separate config file, but at the top-level now.

[nfriedly]

What do you think about changing req.rateLimit.current to req.rateLimit.used in v7 to better align with the terminology in the IETF draft?

We could keep the old one around possibly with Object.defineProperty() to make it not show up, e.g.

Object.defineProperty(rateLimitInfo, 'current', { configurable: false, enumerable: false, get: function() { return this.used } })

That would allow code calling req.rateLimit.current to continue working, but hide it from iteration and JSON.stringify() and the like. We could even throw a validation warning in the getter if we wanted. (Although, unfortunately, that would fire in the cases where it doesn't break anything, and not in the cases where it does, so maybe not worth it.)

[gamemaker1]

Sounds good!

[gamemaker1]

Do you want to put this in v7 or v6.11?

[nfriedly]

Probably v7, because it is a breaking change in some scenarios.

[nfriedly]

Ok, there's only two items left on the v7 checklist, and I think we can skip them both:

• maybe suppress max=0 warning if a custom handler is set (this will be the case for express-slow-down and could conceivably be helpful elsewhere)

Now that specific validations can be disabled by configuration, I don't think this is as important.

• Maybe: switch esbuild to tsc to use decorators?

This was primarily to make the validations changes I had in mind easier, but I ended up working them out without this. So I think we can skip it and stick with what works.

I think we're ready, what do you think?

[gamemaker1]

🚀

[gamemaker1]

Done! Yay!