Skip to content
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.

Sign up for GitHub

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

Jump to bottom

docs - createCookieSessionStorage says commitSession not needed #9444

Open
penx opened this issue May 16, 2024 · 3 comments · May be fixed by #9445
Open

docs - createCookieSessionStorage says commitSession not needed #9444

penx opened this issue May 16, 2024 · 3 comments · May be fixed by #9445
Labels
docs

Comments

@penx
Copy link
Contributor

penx commented May 16, 2024 •
edited
Loading

The docs for createCookieSessionStorage state:

With other session storage strategies you only have to commit it when it's created

https://remix.run/docs/en/main/utils/sessions#createcookiesessionstorage

When we moved from createCookieSessionStorage to another storage solution (vercel kv), we followed this advice and removed calls to commitSession (assuming it was not needed, and that session.set would be all that is required) but it resulted in sessions not being saved.

Elsewhere in the docs for createSessionStorage:

updateData will be called from commitSession

https://remix.run/docs/en/main/utils/sessions#createsessionstorage

Which seems to imply that anything based on createSessionStorage will require a call to commitSession in order to update the session, and I'm fairly sure explains why we would have bugs when removing calls to commitSession.

I think the docs for createCookieSessionStorage should instead state

With other session storage strategies you only have to send an updated cookie in the request header when it's created

I'll raise a PR to the docs for this.
@penx penx changed the title Documentation is likely incorrect Documentation for createCookieSessionStorage is likely incorrect May 16, 2024
@penx penx changed the title Documentation for createCookieSessionStorage is likely incorrect docs - createCookieSessionStorage says commitSession not needed May 16, 2024
penx added a commit to penx/remix that referenced this issue May 16, 2024
@penx
Update sessions.md - note about commitSession
feb9645 
Fixes remix-run#9444
@penx penx linked a pull request May 16, 2024 that will close this issue
Open
1 task
@kiliman
Copy link
Collaborator

kiliman commented May 16, 2024

Yes, the documentation needs to be corrected. For the session API, all session storage providers require you to call commitSession after mutation.

I think what the documentation was alluding to was you don't have to return a set-cookie header in the response after every commit, just the initial one.

In cookie session storage, the session data is stored entirely in the cookie, so any changes require you to send an updated cookie via the set-cookie header.

However, in other storage providers like Cloudflare KV or file-based sessions, the only thing stored in the cookie is the session ID. You would send that id in the set-cookie header on creation. The session ID is used to look up the session in the storage. Since the session id never changes, you don't need to keep sending a set-cookie header.

@penx
Copy link
Contributor Author

penx commented May 16, 2024

@kiliman thanks, I raised a PR here along those lines. Let me know if you have a suggestion on wording
#9445

@akomm
Copy link

akomm commented May 18, 2024 •
edited
Loading

Might be related with this issue? Its about memory / custom storage, but looks like same sort of problem with the behavior and documentation:
#9338

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

docs(sessions): Correction about commitSession in non-cookie sessions penx/remix
4 participants
@kiliman @machour @penx @akomm