docs(utils/sessions): improve docs #7247
Conversation
|
MichaelDeBoey
changed the title
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
aaronadamsCA
force-pushed
the
update-sessions-docs
branch
from
b940a40
to
48559b3
Compare
aaronadamsCA
force-pushed
the
update-sessions-docs
branch
from
48559b3
to
c6f6325
Compare
docs/utils/sessions.md
Outdated
| The downside is that you have to `commitSession` in almost every loader and action. If your loader or action changes the session at all, it must be committed. That means if you `session.flash` in an action, and then `session.get` in another, you must commit it for that flashed message to go away. With other session storage strategies you only have to commit 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). | ||
| However, cookie-based sessions may not exceed the browser's cookie size limit, typically 1024 bytes per cookie value (and 4 KB total for all cookies). | ||
|
|
||
| The other downside is that you need to update the `Set-Cookie` header in every loader and action that modifies the session. With other strategies you only need to set the session cookie once, because it doesn't actually store any session data, just the key to find it elsewhere. |
There was a problem hiding this comment.
That means if you
session.flashin an action, and thensession.getin another, you must commit it for that flashed message to go away.
^ Did we lose this nuance from above? This feels important to call out explicitly since get is not an obvious mutation (even though I know we have a flash section below).
| The other downside is that you need to update the `Set-Cookie` header in every loader and action that modifies the session. With other strategies you only need to set the session cookie once, because it doesn't actually store any session data, just the key to find it elsewhere. | |
| The other downside is that you need to update the `Set-Cookie` header in every loader and action that modifies the session (this includes reading a flashed session value). With other strategies you only need to set the session cookie once, because it doesn't actually store any session data, just the key to find it elsewhere. |
There was a problem hiding this comment.
I've made this change.
docs/utils/sessions.md
Outdated
|
|
||
| The downside is that you have to `commitSession` in almost every loader and action. If your loader or action changes the session at all, it must be committed. That means if you `session.flash` in an action, and then `session.get` in another, you must commit it for that flashed message to go away. With other session storage strategies you only have to commit 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). | ||
| However, cookie-based sessions may not exceed the browser's cookie size limit, typically 1024 bytes per cookie value (and 4 KB total for all cookies). |
There was a problem hiding this comment.
I think the original 4k is accurate here. It's 4k per total cookie length (including name/attrs) and 1k per attribute value per Chrome's docs and the spec also states 4k but doesn't say anything about the attribute length limit.
I did a quick test and as expected this is the largest cookie I can set before it gets ignored:
"Set-Cookie": `test=${new Array(4092).fill("a").join("")}`,
Let's just leave it at 4k and not say anything about the 1k attribute limit.
The only "combined" cookie limits I've run into is ~15k in GCP before you start getting 429 errors.
There was a problem hiding this comment.
| However, cookie-based sessions may not exceed the browser's cookie size limit, typically 1024 bytes per cookie value (and 4 KB total for all cookies). | |
| However, cookie-based sessions may not exceed the browser's cookie size limit - typically 4k bytes per cookie. |
There was a problem hiding this comment.
Thank you. I've made this change too, with an extra detail I did not know before:
However, cookie-based sessions may not exceed browser cookie size limits of 4k bytes. If your cookie size exceeds this limit,
commitSession()will throw an error.
This is good to call out, since it clarifies that exceeding the limit will result in an app crash. I was happy to see this safeguard present in the library—better to log an app crash than to have a page rejected by a browser.
docs/utils/sessions.md
Outdated
|
|
||
| For file-backed sessions, use `createFileSessionStorage()`. File session storage requires a file system, but this should be readily available on most cloud providers that run express, maybe with some extra configuration. | ||
|
|
||
| The advantage of file-backed sessions is that only the session ID is stored in the cookie while the rest of the data is stored in a regular file on disk, ideal for sessions with more than 4kb of data. | ||
| The advantage of file-backed sessions is that only the session ID is stored in the cookie while the rest of the data is stored in a regular file on disk, ideal for sessions with more than 1024 bytes of data. |
There was a problem hiding this comment.
| The advantage of file-backed sessions is that only the session ID is stored in the cookie while the rest of the data is stored in a regular file on disk, ideal for sessions with more than 1024 bytes of data. | |
| The advantage of file-backed sessions is that only the session ID is stored in the cookie while the rest of the data is stored in a regular file on disk, ideal for sessions with more than 4k bytes of data. |
MichaelDeBoey
added
needs-response
|
@aaronadamsCA Are you still interested in getting this PR merged? |
MichaelDeBoey
added
the
needs-response
|
Whoops, thanks for the bump, @MichaelDeBoey! I completely forgot about this. I've updated the branch and made @brophdawg11's requested changes. The only thing that needs checking is the line numbers in the code block, I'm not sure how to preview those to ensure the correct lines are being highlighted. |
github-actions
bot
removed
the
needs-response
See #7225 (comment) for details. In the absence of a larger rewrite I think this should substantially improve clarity and reduce confusion.
Key bits include:
commitSession(), you're losing your changes.Set-Cookie, you're still losing your changes.app/session.server.tsoverapp/session.ts.The one more opinionated thing I did as well was to remove the lengthy example under
session.flash(), because there is already an example of the same thing earlier in the doc.Please let me know if there's anything else I can do to help ship this, so that I can stop making the same mistakes over and over every time I read this doc page. Thanks!
@brophdawg11