-
-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: full sessions support w/ environment api #14696
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
- create virtual module for cloudflare config (sessionKVBindingName) - handler dynamically accesses binding from env via virtual module - remove hardcoded SESSION binding name, now configurable - remove unused __env__ global - add test fixture with sessions form - sessions now fully integrated with astro environment api
|
|
@copilot can you run lint |
* Initial plan * fix: organize imports and formatting in handler.ts Co-authored-by: matthewp <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: matthewp <[email protected]>
|
@matthewp what I fail to understand is what we're fixing/making better. Can you highlight that, please? |
ematipico
left a comment
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.
Tested locally, it works
| <h1>Sessions</h1> | ||
| <h2>Cart</h2> | ||
| <p> | ||
| <a href="/checkout">🛒 {cart?.length ?? 0} items</a> |
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.
Checkout doesn't exist. Tested locally and it gives 404
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.
This was already in here, look at the diff. I think you added it :D
I only added to the demo to confirm that it was indeed saving to the cache.
| "kv_namespaces": [ | ||
| { | ||
| "binding": "SESSION", | ||
| "id": "SESSION" | ||
| } |
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.
Could you let me know where you found the docs for this? I couldn't make it working
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.
|
@ematipico The previous implementation of sessions depended on globals which of course we can't rely on any more. So the fix is to provide the session on the config object, here: https://github.com/withastro/astro/pull/14696/files#diff-2f4ffbe3ae5b73f769512941577c209c575ce394e67c9f0f8f28711d056d9eebR43-R48 |
Changes
virtual:astro-cloudflare:config) to expose configurable bindingsSESSIONbinding name, now fully configurable viasessionKVBindingName__env__global variableTesting
Tested with Cloudflare Pages local dev server:
Astro.session?.get()Astro.session?.set()Docs
No docs changes needed - sessions functionality documented in existing guide. Custom binding name option already documented in integration options.