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

feat(sessions): implement ttl and flash #12693

Merged
merged 8 commits into from
Dec 10, 2024
Merged

Conversation

ascorbic
Copy link
Contributor

@ascorbic ascorbic commented Dec 9, 2024

Changes

Adds support for session data expiry. This includes:

  • a new ttl config option
  • support for a ttl argument to set
  • support for session.flash(), which expires the data after the next request

Testing

Added unit tests

Docs

Copy link

changeset-bot bot commented Dec 9, 2024

⚠️ No Changeset found

Latest commit: a55fedb

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.

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) docs pr labels Dec 9, 2024
@ematipico
Copy link
Member

Can you update the RFC with the relative information, so we can review the PR?

@ascorbic
Copy link
Contributor Author

ascorbic commented Dec 9, 2024

@ematipico the RFC has been updated

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise, I would add more test cases, especially for middleware and rewrites. I also left some questions in the RFC around the .flash.

Comment on lines +2 to +3
Astro.session.flash('flash-value', `Flashed value at ${new Date().toISOString()}`);
return Astro.redirect('/');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add more cases, such as:

  • usage with Astro.rewrite
  • usage with the next("/some-route") (the other form of rewrite): see if the value changes when the next middleware function read from .flash
  • usage with multiple middleware functions e.g. sequence(fn1, fn2)

@@ -138,10 +147,19 @@ export class AstroSession<TDriver extends SessionDriverName = any> {
this.#cookieSet = true;
}
this.#data ??= new Map();
this.#data.set(key, value);
const lifetime = ttl ?? this.#config.ttl;
const expires = typeof lifetime === 'number' ? Date.now() + lifetime * 1000 : lifetime;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment that explains the 1000?

@ascorbic ascorbic merged commit f45962d into astro-dot-session Dec 10, 2024
4 checks passed
@ascorbic ascorbic deleted the session-ttl branch December 10, 2024 15:58
ematipico added a commit that referenced this pull request Dec 18, 2024
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs pr pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants