-
Notifications
You must be signed in to change notification settings - Fork 30
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
base: main
Are you sure you want to change the base?
Conversation
// The required options depend on the driver | ||
options: { | ||
base: "session", | ||
host: process.env.REDIS_HOST, | ||
password: process.env.REDIS_PASSWORD, | ||
}, |
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.
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?
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.
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.
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.
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?
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.
// 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.
proposals/0054-sessions.md
Outdated
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. |
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'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.
// Required: the name of the Unstorage driver | ||
driver: "redis", |
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 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?
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.
What do you mean by shared configuration?
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.
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
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.
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.
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.
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.
proposals/0054-sessions.md
Outdated
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 |
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.
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.
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.
The "on-demand rendered" is meant to apply to "pages and API endpoints". I'll reword it to be less ambiguous.
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. |
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.
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?
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.
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.
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.
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()
})
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 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.
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.
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?
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 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.
There are a few that don't make sense in serverless, like fs
or the LRU cache (anything in memory).
I would like to improve that DX if possible. Why can't an adapter accept a predefined list of supported storage?
It would make more sense if they provided a list of drivers not supported. Like serverless and in-memory. Although that's not even guaranteed to not work either as a serverless function can stay alive an unknown amount of time.
Since you can write your own drivers this wouldn't work anyways; you could write your own in-memory driver and the adapter would have no way of knowing if it works or not.
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.
Yeah, true it should work if you set the credentials manually
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 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!
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.
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.
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.
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.
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.
Summary
An
Astro.session
primitive, with pluggable storage backends.Links