-
Notifications
You must be signed in to change notification settings - Fork 2.7k
docs(utils/sessions): improve docs #7247
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
Conversation
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
b940a40
to
48559b3
Compare
48559b3
to
c6f6325
Compare
@aaronadamsCA Are you still interested in getting this PR merged? |
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. |
Thanks for the merge @brophdawg11, though as I read these changes back a year and a half later, I desperately hope React Router middleware can at last solve all these race-condition pains. 😅 |
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.ts
overapp/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