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.

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

penx
Copy link
Contributor

@penx penx commented May 16, 2024

Closes: #9444

  • Docs
  • n/a Tests

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 (createKvSessionStorage from @vercel/remix), 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.

Copy link

changeset-bot bot commented May 16, 2024

⚠️ No Changeset found

Latest commit: 149997c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

docs/utils/sessions.md Outdated Show resolved Hide resolved
@penx penx changed the title Update sessions.md - note about commitSession docs(sessions): note about commitSession May 16, 2024
@penx penx changed the title docs(sessions): note about commitSession docs(sessions): correction about commitSession on non-cookie sessions May 16, 2024
@penx penx changed the title docs(sessions): correction about commitSession on non-cookie sessions docs(sessions): Correction about commitSession on non-cookie sessions May 16, 2024
@penx penx changed the title docs(sessions): Correction about commitSession on non-cookie sessions docs(sessions): Correction about commitSession in non-cookie sessions May 16, 2024
docs/utils/sessions.md Outdated Show resolved Hide resolved
docs/utils/sessions.md Outdated Show resolved Hide resolved
docs/utils/sessions.md Outdated Show resolved Hide resolved
docs/utils/sessions.md Outdated Show resolved Hide resolved
docs/utils/sessions.md Outdated Show resolved Hide resolved
@penx penx requested a review from machour May 17, 2024 09:36
docs/utils/sessions.md Outdated Show resolved Hide resolved
@machour machour requested a review from brophdawg11 May 17, 2024 12:58
@brophdawg11 brophdawg11 added docs feat:sessions Sessions related issues labels May 20, 2024
@penx
Copy link
Contributor Author

penx commented Jun 14, 2024

@brophdawg11 anything needed to get this merged? 🙏

Comment on lines +280 to +286
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.
Copy link
Contributor

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:

Suggested change
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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed docs feat:sessions Sessions related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs - createCookieSessionStorage says commitSession not needed
3 participants