fix(types): allow middleware props to be optional when specified in global config

[GalacticHypernova]

Types of changes

• Bug fix (a non-breaking change which fixes an issue)
• New feature (a non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Description

Allows to specify multiple properties only partially while keeping the rest in default values

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes (if not applicable, please state why)

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nuxt-security	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 27, 2024 8:09pm

[vejja]

I’m not sure they can be optional.
Isn’t it only the case if the default config is used?

[GalacticHypernova]

The default config is used regardless in module.ts, so I assume it can be optional?
https://github.com/Baroshem/nuxt-security/blob/main/src/module.ts#L80

[vejja]

Not if the user deactivates them with ‘false’
Which then might become an issue at route level?

[GalacticHypernova]

Well, does the default config per route not work with route level configuration? If a person doesn't specify 'false' at a certain route, won't the default configuration for that route be the one with the default values? If not, then there's the question of whether we want to sacrifice dx for global config to ensure proper values at route level config

[vejja]

The routes are resolved recursively so if at any level the feature is deactivated with false, all routes nested within that level will not inherit any default values. Worth double-checking, but I’m pretty sure that this is what defu will do.

Also if the user changes a default value in the global options (or at any route level), all nested route rules (below that route) will inherit this custom value and not the default value.

Even if we changed the code to modify this behaviour, wouldn't we end up in a situation where we have inconsistent behaviour across features ? e.g.

• for all features, every field value is erased after false
• except for the rateLimiter and requestSizeLimiter features, which have a different behaviour (set to some other value).

[GalacticHypernova]

Lastly, wouldn't we end up in a situation where we have inconsistent behaviour across features ?

I mean, the throwError field is already optional and I haven't seen any issues with that in any of the middleware?

Also if the user changes a default value in the global options, he would then expect all nested routes to inherit his custom value, not the defaultConfig value that would be set with this proposal.

Well, it would inherit from the global config but would be overridden by per-route config, wouldn't it? That way if someone changes the default value in global config, per route config would inherit from the global config, rather than the default one.

[vejja]

The problem only arises with route-level configurations.
If we want the user to be able to deactivate the feature for a certain route level, then we need to give him the ability to reactivate it for a nested route below. At this point there is no definition of a fallback default anymore.

[GalacticHypernova]

Interesting... how about adding specific type overload for per route config that would require all required props?

[vejja]

Absolutely, great idea

[GalacticHypernova]

I made the route-specific config have all the required props, so now it shouldn't cause any issues. Although the throwError would also be required in that case. Should this one stay optional?

[vejja]

Hey @GalacticHypernova, looks good here
I'm wondering whether I could request 2 additional changes

• merge from 2.0.0-rc.1 instead of main : this is because the RateLimiter type has changed
• use only Required instead of RequiredOptional : this is because the new RateLimiter type in the route rules already eliminates some fields, so for clarity it would be easier to read (I think this request will become obvious when you rebase from 2.0.0-rc.1)

In addition, throwError can remain optional because the implicit default is assumed to be false

[GalacticHypernova]

merge from 2.0.0-rc.1 instead of main : this is because the RateLimiter type has changed

I don't think I have access to rebase the PR unfortunately, is there a way you could do it please?

use only Required instead of RequiredOptional : this is because the new RateLimiter type in the route rules already eliminates some fields, so for clarity it would be easier to read (I think this request will become obvious when you rebase from 2.0.0-rc.1)

I accidentally recreated Required through RequiredOptional, forgot the type existed

[GalacticHypernova]

Updated it to comply with the RC branch. Anything else you think I should add?

[vejja]

throwError is now required in route rules, but maybe it could stay optional ?
Also would you mind verifying if the docs need to be updated for throwError because I said default was false but actually I think I was wrong

[GalacticHypernova]

throwError is now required in route rules, but maybe it could stay optional ?

Done, it should now stay optional.

because I said default was false but actually I think I was wrong

The default is true (in the docs)