Closed GalacticHypernova closed 4 weeks ago
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 |
I’m not sure they can be optional. Isn’t it only the case if the default config is used?
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
Not if the user deactivates them with ‘false’ Which then might become an issue at route level?
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
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.
false
rateLimiter
and requestSizeLimiter
features, which have a different behaviour (set to some other value).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.
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.
Interesting... how about adding specific type overload for per route config that would require all required props?
Absolutely, great idea
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?
Hey @GalacticHypernova, looks good here I'm wondering whether I could request 2 additional changes
RateLimiter
type has changedRequired
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
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
Updated it to comply with the RC branch. Anything else you think I should add?
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
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)
Types of changes
Description
Allows to specify multiple properties only partially while keeping the rest in default values
Checklist: