-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
feat: prototype session support #12471
Conversation
|
* feat: implement sessions * Add middleware * Action middleware test
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 did a review of the code, however I am not ready to approve it because there are some things that aren't clear to me yet
a5d977c
to
60ad014
Compare
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.
Code-wise it looks good to me. I still think we should update the RFC and explain the concept of sparse data. I didn't know what it was, and it's possible other people don't. I left a couple of questions.
// Whether the session is dirty and needs to be saved. | ||
#dirty = false; |
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.
It's possible we have a different concept of "dirty." In my head, dirty means "not pristine; there's some data." However, when we calldeleteSafe
, its value is set to false,
Is it possible I misunderstood things?
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.
Dirty means the data has changed in some way
cookie: z | ||
.union([ | ||
z.object({ | ||
name: z.string().optional(), |
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.
How will the name of the driver mapped to the unstorage drivers? As far as I understood from the unstorage
docs, drivers aren't strings 🤔
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 copied the way Nitro does it, because they also need serialisable config to load the drivers via strings. https://github.com/withastro/astro/pull/12471/files#diff-a43699ec38288009ad632121f37db32347eee67258113b3a8008e82ab8a259d6R365-R373
* wip: experimental sessions * feat: adds session options (#12450) * feat: add session config * chore: add session config docs * Fix * Expand doc * Handle schema * Remove example * Format * Lock * Fix schema * Update packages/astro/src/types/public/config.ts Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca> * Update packages/astro/src/types/public/config.ts Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca> * Add link to Sessions RFC in config.ts * Move session into experimental --------- Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca> * Lock * feat: prototype session support (#12471) * feat: add session object * Add tests and fix logic * Fixes * Allow string as cookie option * wip: implement sessions (#12478) * feat: implement sessions * Add middleware * Action middleware test * Support URLs * Remove comment * Changes from review * Update test * Ensure test file is run * ci: changeset base * ci: exit from changeset pre mode * Lockfile * Update base * fix: use virtual import for storage drivers (#12520) * fix: use virtual import for storage drivers * Don't try to resolve anythign in build * Fix test * Polyfill node:url * Handle custom drivers directly * No need for path * Update packages/astro/src/core/session.ts Co-authored-by: Emanuele Stoppa <my.burning@gmail.com> --------- Co-authored-by: Emanuele Stoppa <my.burning@gmail.com> * Fix jsdoc * fix: set default storage path * Update changeset config for now * Revert config workaround * Lock * Remove unneeded ts-expect-error directive * fix: [sessions] import storage driver in manifest (#12654) * wip * wip * Export manifest in middleware * Changeset conf * Pass session to edge middleware * Support initial session data * Persist edge session on redirect * Remove middleware-related changes * Refactor * Remove vite plugin * Format * Simplify import * Handle missing config * Handle async resolution * Lockfile * feat(sessions): implement ttl and flash (#12693) * feat(sessions): implement ttl and flash * chore: add unit tests * Make set arg an object * Add more tests * Add test fixtures * Add comment * Remove session.flash for now (#12745) * Changeset * Apply suggestions from code review Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com> --------- Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca> Co-authored-by: Emanuele Stoppa <my.burning@gmail.com> Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
Changes
Prototype implementatiopn of sessions.
Testing
Includes lots of unit tests. There is also a fixture with lots of examples but no tests that target it yet.
Docs
Adds new AstroErrors