-
Notifications
You must be signed in to change notification settings - Fork 141
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
Fix TS definition for appSessionSecret #52
Fix TS definition for appSessionSecret #52
Conversation
@@ -23,7 +23,7 @@ interface ConfigParams { | |||
* REQUIRED. The secret(s) used to derive an encryption key for the user identity in a session cookie. | |||
* Can use env key APP_SESSION_SECRET instead. | |||
*/ | |||
appSessionSecret: boolean | string | string[]; | |||
appSessionSecret?: boolean | string | string[]; |
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.
Are there any tests needed around this?
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 don't know but probably not? Are there test you can write for a JS library TS definition file?
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.
Not so much the definition file but I meant the fact that the property is now optional. What happens when it's not supplied and and environment variable isn't set?
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.
Then you'll get a validation error on startup.
@@ -23,7 +23,7 @@ interface ConfigParams { | |||
* REQUIRED. The secret(s) used to derive an encryption key for the user identity in a session cookie. |
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.
Should this now read OPTIONAL
?
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.
No, it's still required but you can set the value in your env and not in the config object.
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.
Hmm, feels weird to have an optional property here but then say it's required in the docs. Wouldn't that confuse people?
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.
The application needs a value, that part is a requirement, but you can set it in the config object (which the TS defs affect) or you can set it in an env variable and skip the config key. Either way it is used by the app.
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.
Description
Fix TS definition for
appSessionSecret
config key, should be optional as it can be defined in an env var.References
Related to #46