-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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: adds session options #12450
feat: adds session options #12450
Conversation
|
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.
So many cool new features coming! 🔥
Just left some thoughts below for your consideration!
* <a href="/checkout">🛒 {cart?.length ?? 0} items</a> | ||
* | ||
* ``` | ||
* |
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.
Link to RFC to learn more, maybe?
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.
Yeah, I'll do that once I have the stage 3 PR up
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.
Done!
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
@@ -714,6 +733,13 @@ export function createRelativeSchema(cmd: string, fileProtocolRoot: string) { | |||
}); | |||
} | |||
} | |||
if (session && !experimental.sessions) { |
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 it would be better to just have it under experimental
, and move it to root once it becomes stable
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.
Still don't know what's the best thing there.
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 that I get a say in the matter, but from a docs perspective, we prefer when everything related to an experimental feature is contained in its own experimental config. 😅
Especially going forward, each experimental feature will have its own page, separate from configuration reference. If everything is contained, it's nice to document all in one experimental feature flag, and we don't have to send people separately to configuration reference for configuring experimental things, and it's easier to know you've got the feature set up when it's all together.. (And, we get to keep experimental content separate from stable, which is more helpful for readers to keep things straight.)
And, separate page for experimental feature means you get to avoid JSDoc and just make normal docs PRs to update... so... does that tempt you??? 😄
On the other side, I think Ema said from a code maintenance or testing perspective, it's easier to work on the code separately from the flag. There's also the advantage of this showing up in the editor. (But then, it shows up for people who also don't have the flag enabled, too. Is that advertising or confusing?) So really, both sides have their benefits.
We can certainly handle documenting either way, and for sure don't want to suggest something difficult to work on or maintain. But if we got to choose, we'd keep the experimental configs within the experimental flag.
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.
You absolutely do get a say! I think I am swayed towards moving it all into experimental
now, and replace the boolean with the object. The fact it's a new top-level option is the difference I think.
*/ export interface AstroUserConfig<TLocales extends Locales = never> { | ||
*/ export interface AstroUserConfig< | ||
TLocales extends Locales = never, | ||
TSession extends SessionDriverName = never, |
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'm glad the generic defineConfig
is used already :)
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.
Yeah, it works great!
Changes
Adds experimental flag for sessions, and top-level
session
optionsTesting
Docs