Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow configuring headers in runtime #298
feat: allow configuring headers in runtime #298
Changes from 6 commits
65fc0d1
da8461a
e242ccc
7207581
6e41f10
1827273
a062371
6acf660
c58491a
10c6161
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: I wonder if this should actually be enabled by default. I think that majority of the users will just use the nuxt.config.ts config or route rules while this runtime configuration is a bit of an edge case (at least IMO).
With these kind of configuration, I usually mark them as optional and disabled by default (to not ship unused code to other users).
So, if you want to have a runtime configuration, just set this config value
security.runtimeHooks = false
and it will work for you.WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i didn't remove this since the removal of the
runtimeHooks
option@harlan-zw think we don't really need this option to always have a runtime configuration of headers. So should we set back
runtimeHooks
in the options ?If we set back
runtimeHooks
option, i think it should be default to false. Then if true, we'll add the nitro pluginThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally would recommend to add an option and make it disabled by default to not add additional code for users that may not use it.
So add an option but make it disabled by default and make a note in the docs that in order to use this feature, set this config option to true and then, call the hook (like you have explained in the docs)
I think this delivers the functionality to the user that may need it without forcing every user of the module to have some dead code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use the module name to avoid conflicts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i used security because it's the module
configKey
, but yeah we can change it tonuxtSecurity
!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if ends users will use this context object it could make sense with this name, otherwise if it's just internal module name is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would stick to
security
instead ofnuxt-security
purely because this is a config key for both nuxt.config.ts file and for route rules so it would be better IMHO to keep this consistent :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@huang-julien is it required to turn off nonces and content security policy headers here?
When I keep
contentSecurityPolicy
headers enabled here, the headers set in the hook are not applied it seems.When I disable
contentSecurityPolicy
headers here, the headers set in the hook are applied but without{{nonce}}
replacement. Is that intended?