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

Sessions #1055

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Sessions #1055

wants to merge 5 commits into from

Conversation

ascorbic
Copy link
Contributor

Summary

An Astro.session primitive, with pluggable storage backends.

---
// src/components/CartButton.astro

const cart = await Astro.session.get('cart');
---
<a href="/checkout">🛒 {cart?.length ?? 0} items</a>
// src/pages/api/addToCart.ts
import type { APIContext } from "astro";

export async function POST(req: Request, context: APIContext) {
  const cart = await context.session.get("cart");
  cart.push(req.body.item);
  await Astro.session.set("cart", cart);
  return Response.json(cart);
}

Links

@florian-lefebvre florian-lefebvre mentioned this pull request Nov 18, 2024
proposals/0054-sessions.md Outdated Show resolved Hide resolved
Comment on lines +192 to +197
// The required options depend on the driver
options: {
base: "session",
host: process.env.REDIS_HOST,
password: process.env.REDIS_PASSWORD,
},
Copy link
Member

Choose a reason for hiding this comment

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

If there are drivers that don't require options, don't options become optional? How do we validate options against the driver? Is it possible to do this at the schema level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, options are optional. We don't have schemas for the options, but we have types, and those do correctly handle the different driver options.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I read // The required options and thought that they were required. So they are optional, but needs to be provided based on the driver.
Will we throw an error if those options are missing for specific drivers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// The required options depend on the driver means some options will or will not be required, depending on the driver chosen.

The driver will throw the error if required options are missing.

Comment on lines 175 to 177
All configuration is optional if the adapter has built-in support. This is the
case for the Node adapter, which uses filesystem storage by default, and is
expected to be the case for most adapters.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this should be part of the proposal, specifically the part when we talk about specific adapters. Or, if we really need to, we should try to actually to be as generic as possible because we don't know what and how the specific adapters will behave.

Comment on lines +190 to +191
// Required: the name of the Unstorage driver
driver: "redis",
Copy link
Member

Choose a reason for hiding this comment

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

I have mixed feelings about using driver in a "shared" configuration. What if the redis driver isn't supported by a specific adapter or any other adapter? How can we tell the user that they can't use a certain adapter with certain driver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by shared configuration?

Copy link
Member

@ematipico ematipico Nov 19, 2024

Choose a reason for hiding this comment

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

With "shared", I meant that If we define a driver here, I would assume - as a user - that this driver will work regardless of any adapter used by. So it's a configuration "shared" by core and the adapter at play

Copy link
Contributor

Choose a reason for hiding this comment

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

There's not such a tight relationship between an adapter and a driver. An adapter doesn't "support" a driver directly. It's more correct to say that some adapters do or do not support:

  • Persistent memory
  • FS access
  • TCP sockets

So for your example, it's not that some adapters support redis, it's that redis works over TCP sockets and some adapters may or may not support opening sockets. But we don't have such a feature map to know those sorts of requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For any drivers that we aren't automatically enabling (so, basically all of them except fs) I think we need to delegate all the documentation to Unstorage, at least in the short term, and that includes which platforms are support. Anything else would be too hard to keep up to date. In future we may decide we want to own the drivers ourselves, but right now I think the idea is to rely on Unstorage wherever possible. If we need changes then we can contribute them upstream.

Because of the variety of environments to which Astro can be deployed, there is
no single approach to storage that can be relied upon in all cases. For this
reason, adapters should provide default session storage drivers where possible.
Sessions are only available in on-demand rendered pages and API endpoints, so
Copy link
Member

Choose a reason for hiding this comment

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

and API endpoints

This is misleading, because endpoints that look like src/pages/schema.json.js are run during the build, they create a physical file called src/pages/schea.json and they aren't run in SSR anymore.

The endpoint needs to be on-demand, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "on-demand rendered" is meant to apply to "pages and API endpoints". I'll reword it to be less ambiguous.

proposals/0054-sessions.md Outdated Show resolved Hide resolved
Comment on lines 264 to 268
The adapter or integration is responsible for ensuring that they do not
overwrite any user-defined driver configuration. Adapters may choose to accept
their own configuration options which they can apply to the storage driver where
needed. Adapters may provide a storage driver for use in development, or rely on
the built-in node adapter which is provided by the dev server.
Copy link
Member

Choose a reason for hiding this comment

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

We should really highlight what would happen if drivers go in conflict, and if so how users/developers can do that. Is Astro responsible for that? Is the adapter for that? Is the user responsible for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There can only be one driver, so I don't think there's an issue with conflicts there. Adapters are meant to prefer the user-defined drivers, though I don't think this is something we can enforce when adapters can do what they want to the config.

Copy link
Member

@ematipico ematipico Nov 19, 2024

Choose a reason for hiding this comment

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

Here's a concrete example that comes to mind

A project with a Cloudflare adapter that attempts to use a Netlify blob. What would happen in this occasion? How does Astro behave? It isn't clear to me from the RFC:

import cloudflare from "@astrojs/cloudflare"
import {defineConfg} from "astro/config"

defineConfig({
	driver: 'netlify-blobs', // should use nstorage/drivers/netlify-blobs
	adapter: cloudflare()
})

Is it possible to have such a combination? If so, how? It doesn't seem likely

In another case, the use of an adapter that doesn't allow FS storage:

import cloudflare from "@astrojs/cloudflare" // let's assume `node:fs` isn't compatible with cloudflare
import {defineConfg} from "astro/config"

defineConfig({
	driver: 'fs',
	adapter: cloudflare()
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need to do checks for people doing things that are clearly not allowed on that platform. Why would somebody choose Netlify blobs if they're not using Netlify? This isn't something we're doing automatically and hiding from them: this is equivalent to somebody choosing the Netlify adapter when they're deploying to Cloudflare, or using fs in a page on a site deployed to Cloudflare.

Copy link
Member

Choose a reason for hiding this comment

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

Why would somebody choose Netlify blobs if they're not using Netlify?

Mistakes, junior people who don't know very well what they're learning, possible differences between local behaviour and production behaviour, etc.

I would like to improve that DX if possible. Why can't an adapter accept a predefined list of supported storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, true it should work if you set the credentials manually

Copy link
Member

Choose a reason for hiding this comment

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

@matthewp

I don't know if its true that you can't use the netlify-blobs driver with Cloudflare. If netlify-blobs operates over HTTP then more than likely it would work.

I don't know either, it was just an example to explain what I meant.

It would make more sense if they provided a list of drivers not supported

That's a step forward!


@ascorbic

I'd really like to avoid coupling us so closely to the Unstorage driver implementation. One of the design goals for this is to avoid needing to maintain them. If we did that, we'd need to update all adapters whenever they add a new driver, and theoretically audit them all manually whenever that happens.

I understand that, however we need to strive towards a decent enough DX and UX for our users. Here are some considerations:

  • We are responsible for the libraries we use as a service (see sharp, drizzle, etc.), and we always try to create a certain layer of compatibility for our users.
  • Since we don't state which library we rely on (rightfully), Astro is free to change the underneath library (if it ever happens) without changing the user facing APIs. For images, for example, we remove squoosh, but the APIs are unchanged - AFAIK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a bigger question then. If we want to fully abstract away unstorage to the extent that we don't need to mention it then we need to document every driver ourselves. Now that may be a sensible thing to do, but it will mean we need to decide on a more limited list of ones that we will support.

Copy link
Contributor

Choose a reason for hiding this comment

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

We talked about this on a call, but I'll restate that we can't ensure that a user only uses compatible libraries with Astro. There are a number of places where a user might use a library that doesn't work:

  • Remark plugins
  • Vite plugins
  • Framework components

There's a tradeoff between protecting the user from mistakes and giving them the power to do what they want. I think this is a case where there's a large amount of compatibility already, and users will naturally use drivers that make sense for their host anyways. Adding restrictions would make the API less powerful and unnecessarily prevent legitimate use-cases.

Copy link

Choose a reason for hiding this comment

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

unstorages raison d’être is to be the uncoupled interface and provide many implementations. It's used successfully by nitro and I can attest for its ergonomics. I don't think there's a compelling reason to reimplement it just to own the namespace it comes from

Regarding saving devs from themselves, obviously it comes from a good place but it's just a wild goose chase. The higher the guard rails, the more convoluted the hacks people would put in place to skip them. If a junior person who doesn't know very well what they're learning make a mistake then they would learn "the hard way". I personally believe that providing good descriptive errors is much more educational and effective than guardrails. Generally this is something astro is very good at, so it's not an odd expectation.

The suggested sessions API is already 99% existing. Add unstorage and use Astro.locals, mix in some cookie action and you're golden. The hard part of sessions was the async local context. With an RFC this obvious and implementation this lean it'll be a shame to bikeshed the issue when everyone involved can clearly see the value.

I hope it's ok that I butted in :)

Comment on lines 121 to 122
- Type-safety. This may be added later, but there are questions about the best
way to do this, because of issues with the way that the config is loaded.
Copy link

Choose a reason for hiding this comment

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

Of course can be added later but adding it from the get-go would be nicer IMO.
To propose one option: Optional override in env.d.ts.

Ideally everything could be inferred from the Astro config in some way but in the mean time I think a sufficient alternative would be to allow users to add types to their src/env.d.ts to add explicit types for sessions. Similar to how it is done for Cloudflare types. For example, based on the examples above:

/// <reference path="../.astro/types.d.ts" />
/// <reference types="astro/client" />

declare namespace App {
	interface Sessions {
		cart: Array<string>;
	}
}

Or if long-term things were to get more advanced some kind of schema could be added as an option to the session config, similar to server actions, to both validate data & infer types from.

Copy link

Choose a reason for hiding this comment

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

You must distinguish between typed and type-safe, sessions must be assumed unreliable and partial in the best of circumstances, enforcing it with a schema is just a trap for when the sessions start to skew or expire and suddenly no longer fit. I think either a FormData-like API (as suggested) or a typing that gets Partial-ized on the object is best, though it'll suck a bit ngl

@ascorbic
Copy link
Contributor Author

ascorbic commented Dec 9, 2024

Some suggestions from the API bash:

  • session.flash(): save something to the session just for the next request
  • TTL in session metadata, and a session.gc() to clean old entries
  • --force in CLI clears the session


### `Astro.session.flash(key: string, value: any): void`

Sets the value of the given key in the session. The value can be any serializable type. The value will be deleted after the next request.
Copy link
Member

Choose a reason for hiding this comment

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

How does Astro.sessions.flash differ from Astro.locals? Other than the fact that the latter can contain non-serializable values.

after the next request

The .rewrite API creates a new request, does that mean that this value should disappear? For example, we navigate the page /foo, which then executes Astro.rewrite('/failure'). If pages/failure.astro reads the flash, would there be any value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flash() is persisted across requests. If you set Astro.locals and then redirect, you won't have access to the result. The value passed to flash() is persisted to storage, but is then deleted after the next request.

Copy link
Member

Choose a reason for hiding this comment

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

That's what it's unclear to me:

  • Astro.locals persists across requests, too.
  • It's unclear to me what "next request" means in this case because Astro.rewrite and next() create a new Request object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does Astro.locals persist across requests? If I have a redirect, how would I read the value in the target page?

Copy link
Member

Choose a reason for hiding this comment

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

OK, I understand what you meant. I understood "across requests" differently. Astro.locals doesn't persist.

I don't understand the usage of flash() though. First, it's written that it persists across requests (page navigations), but then it's written that it disappears after the next request (page navigation). It seems like it's similar to Astro.locals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only invalidated by requests that actually read the session, so random image requests won't affect it.

Copy link

Choose a reason for hiding this comment

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

Those can also sneak in, and the worse the network conditions are the more likely it is to happen. I can go into many specific real-world scenarios where this is very likely. Anything with SWR, pings, SSE, any sort of polling, prefetching, navigation, multiple-tabs, timeouts, there are so many reasons for requests to be sent in indeterminate times. This is even more pronounced in authenticated flows since most interactions would check the identity of the requester, these flows are also the most likely to benefit from the session API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flash is an optional API. You can always use a regular session.set if your use case may have extra requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

How is flash implemented in other frameworks that support it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I basically copied the Laravel API.

Sometimes you may wish to store items in the session for the next request. You may do so using the flash method. Data stored in the session using this method will be available immediately and during the subsequent HTTP request. After the subsequent HTTP request, the flashed data will be deleted. Flash data is primarily useful for short-lived status messages:

@wildfiremedia
Copy link

My concern is related to testing fs and running the wrk benchmark tool, as I discovered that my folder became filled with a large number of new sessions, potentially making the system vulnerable to running out of disk space.

Is there a way to limit the folder/disk size allocated for storing sessions?

@bkyerv
Copy link

bkyerv commented Dec 27, 2024

are there any plans for adding built-in support for session signing and verification, similar to Remix's implementation. This would help protect against session tampering and provide developers with secure defaults.

Key considerations:

  • session signing: a mechanism to cryptographically sign session data, ensuring that the session hasn't been modified client-side
  • verification process: add automatic verification of session signatures before processing session data
    key management: provide a way to configure signing keys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants