-
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
docs(sessions): Correction about commitSession in non-cookie sessions #9445
base: main
Are you sure you want to change the base?
Conversation
|
Co-authored-by: Mehdi Achour <[email protected]>
@brophdawg11 anything needed to get this merged? 🙏 |
The downside is that you have to `commitSession` and send a "Set-Cookie" header from every loader and action that changes the session. This means, for example, that if you `session.flash` in an action, and then `session.get` in another, you must commit it for that flashed message to go away. | ||
|
||
This can cause complications if loaders or actions are writing to the same session at the same time. | ||
|
||
With other session storage strategies you only have to send a "Set-Cookie" header when the session is created (the browser cookie doesn't need to change because it doesn't store the session data, just the key to find it elsewhere). | ||
|
||
Note that you still need to call `commitSession()` when you change the session for anything based on `createSessionStorage`, you just don't need to send an updated header. |
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 the confusion in the original docs is that the phrase "commit" is being used when it really means "set header". So we mean to say "with others you don't need to set the header every time" but we instead incorrectly imply that with others scenarios "you don't need to commitSession every time".
What about this slight update to the original wording? Instead of re-iterating that you need to commitSession
after changes (which is only needed because of the incorrect implication that you don't) - we can just remove the misleading implication otherwise and indicate the true difference which relates to the Set-Cookie
header:
The downside is that you have to `commitSession` and send a "Set-Cookie" header from every loader and action that changes the session. This means, for example, that if you `session.flash` in an action, and then `session.get` in another, you must commit it for that flashed message to go away. | |
This can cause complications if loaders or actions are writing to the same session at the same time. | |
With other session storage strategies you only have to send a "Set-Cookie" header when the session is created (the browser cookie doesn't need to change because it doesn't store the session data, just the key to find it elsewhere). | |
Note that you still need to call `commitSession()` when you change the session for anything based on `createSessionStorage`, you just don't need to send an updated header. | |
The downside is that you have to update the cookie via a `Set-Cookie` header in almost every loader and action. If your loader or action changes the session at all, it must be sent back as an updated cookie. That means if you `session.flash` in an action, and then `session.get` in another, you must update it for that flashed message to go away. With other session storage strategies you only have to send the `Set-Cookie` header it when it's created (the browser cookie doesn't need to change because it doesn't store the session data, just the key to find it elsewhere). |
Closes: #9444
The docs for
createCookieSessionStorage
state:https://remix.run/docs/en/main/utils/sessions#createcookiesessionstorage
When we moved from
createCookieSessionStorage
to another storage solution (createKvSessionStorage
from@vercel/remix
), we followed this advice and removed calls tocommitSession
(assuming it was not needed, and thatsession.set
would be all that is required) but it resulted in sessions not being saved.Elsewhere in the docs for
createSessionStorage
:https://remix.run/docs/en/main/utils/sessions#createsessionstorage
Which seems to imply that anything based on
createSessionStorage
will require a call tocommitSession
in order to update the session, and I'm fairly sure explains why we would have bugs when removing calls tocommitSession
.