Conflicting types in the "Unstorage compatibility" example

[ArmandPhilippot]

📚 Subject area/topic

Session Driver API

📋 Page(s) affected (or suggested, for new content)

https://docs.astro.build/en/reference/session-driver-reference/#unstorage-compatibility
https://docs.astro.build/en/guides/sessions/#overriding-the-configuration-at-runtime

📋 Description of content that is out-of-date or incorrect

The last code snippet in Unstorage compatibility triggers TypeScript errors.

• no type on config
• on the return statement:

  Type 'Driver<RedisOptions, any>' is not assignable to type 'SessionDriver'.
    Types of property 'removeItem' are incompatible.
      Type '((key: string, opts: TransactionOptions) => MaybePromise<void>) | undefined' is not assignable to type '(key: string) => Promise<void>'.
        Type 'undefined' is not assignable to type '(key: string) => Promise<void>'.
  

• on tls:

  Object literal may only specify known properties, and 'tls' does not exist in type 'RedisOptions'.
  

The issues on config and ttl are easily fixable. But, the one on return suggests a compatibility issue. And, we can't "fix" that with a cast:

Conversion of type 'Driver<RedisOptions, any>' to type 'SessionDriver' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  Types of property 'removeItem' are incompatible.
    Type '((key: string, opts: TransactionOptions) => MaybePromise<void>) | undefined' is not comparable to type '(key: string) => Promise<void>'.
      Type '(key: string, opts: TransactionOptions) => MaybePromise<void>' is not comparable to type '(key: string) => Promise<void>'.
        Target signature provides too few arguments. Expected 2 or more, but got 1.

• Unstorage type
• Astro type

If I try an absurd workaround:

import type { SessionDriver } from "astro";
import redisDriver, { type RedisOptions } from "unstorage/drivers/redis";

export default function (config: RedisOptions): SessionDriver {
  const unstorageDriver = redisDriver({
    ...config,
    ttl: config.ttl ?? 60 * 60 * 24 * 7, // default to 7 days
  });
  return {
    ...unstorageDriver,
    async removeItem(key) {
      if (unstorageDriver.removeItem) {
        return await unstorageDriver.removeItem(key, {});
      }
      return void 0;
    },
  };
}

It seems other APIs are incompatible. I now get:

Type '{ removeItem(key: string): Promise<void>; name?: string | undefined; flags?: DriverFlags | undefined; options?: RedisOptions | undefined; ... 12 more ...; watch?: ((callback: WatchCallback) => MaybePromise<...>) | undefined; }' is not assignable to type 'SessionDriver'.
  The types returned by 'getItem(...)' are incompatible between these types.
    Type 'MaybePromise<StorageValue>' is not assignable to type 'Promise<any>'.
      Type 'null' is not assignable to type 'Promise<any>'.

So:

• Should we remove the "Alternatively, [...]" part in docs? (or even the whole "Unstorage compatibility" section? I don't know if the first part work on a real project)
• Should the code snippet be updated in some way?
• Should this be an issue in core to find a fix?

cc @florian-lefebvre as you authored that page (#12951), maybe you have ideas on this.

🖥️ Reproduction in StackBlitz (if reporting incorrect content or code samples)

https://stackblitz.com/edit/astro-session-unstorage?file=src%2Fdriver%2Fruntime.ts&on=stackblitz

This contains fixes for config (import RedisOptions) and tls (swap with ttl) and only reproduces the return statement error.

You should see the error in Stackblitz. Otherwise, you can run npm run astro check.

[alexandrosang21]

The return error is not fixable in the snippet, because SessionDriver is not an unstorage
Driver and was never meant to be one. Checked against astro@main (7c4bf1b) and
unstorage@1.17.5.

packages/astro/src/core/session/types.ts:

export interface SessionDriver {
	removeItem: (key: string) => Promise<void>;
	getItem: (key: string) => Promise<any>;
	setItem: (key: string, value: any) => Promise<void>;
}

unstorage's Driver:

getItem: (key: string, opts?: TransactionOptions) => MaybePromise<StorageValue>;
setItem?: (key: string, value: string, opts: TransactionOptions) => MaybePromise<void>;
removeItem?: (key: string, opts: TransactionOptions) => MaybePromise<void>;

Three separate mismatches, which is why chasing them one at a time keeps producing a new
error:

1. removeItem and setItem are optional on Driver, required on SessionDriver
2. every Driver method returns MaybePromise<T>, SessionDriver requires Promise<T>
3. getItem resolves StorageValue, which includes null

So no cast fixes it, and the "absurd workaround" fails for the same reason one level down.

Why the interface is shaped that way

runtime.ts feeds the factory result to unstorage, filling in the parts it does not need
(packages/astro/src/core/session/runtime.ts:476):

this.#storage = createStorage({
	driver: {
		...driver(this.#config.options),
		// Unused methods
		hasItem() {
			return false;
		},
		getKeys() {
			return [];
		},
	},
});

hasItem and getKeys are required on Driver and stubbed here, so SessionDriver is a
deliberately reduced hand-written interface covering only the three methods sessions use. It
is not meant to be structurally compatible with Driver, it is meant to be smaller.

That is consistent with how the built-in drivers work: drivers.ts never produces a
SessionDriver at all, it maps builtinDrivers to { entrypoint, config } and lets the
entrypoint be loaded later. The built-in path and the custom-driver path do not share a type.

Which of your three questions I think this answers

The first half of the section, the { entrypoint, config } form, is fine and matches
drivers.ts. It is only the "Alternatively" snippet, the one that wraps an unstorage driver
and returns it as a SessionDriver, that cannot typecheck.

So this does look like a core question rather than a docs one, and the choice is whether
SessionDriver should widen to accept MaybePromise returns and optional removeItem, which
would make real unstorage drivers assignable and is backwards compatible for anyone already
implementing the strict shape, or whether the docs should stop presenting an unstorage driver
as something you can pass straight through.

I have only verified the type side of that. Whether widening is safe at runtime depends on
createStorage tolerating a missing removeItem, which I have not tested, so I would not want
to assert it.

Two smaller things confirmed while checking, both already in your StackBlitz: tls is not on
RedisOptions and ttl is, and config needs the explicit RedisOptions import.

[florian-lefebvre]

@ArmandPhilippot mmh that's odd. I was pretty sure the types used to be compatible but doesn't look like it from the links you shared. So I think these should be done:

1. Fix config and tls in docs
2. Before fixing in core, see if improvements can be done in unstorage directly. I'll open an issue because I have ideas (EDIT: Properly infer available driver methods unjs/unstorage#805)

[ArmandPhilippot]

Sounds good to me, thanks!

And, to keep track in case we need further documentation updates, the AI ​​PR we received identified another place where we're encountering the same issue: https://docs.astro.build/en/guides/sessions/#overriding-the-configuration-at-runtime

[florian-lefebvre]

Btw I think documenting the workaround is fine (see #14409), but I'd like if there was a note somewhere (maybe not in docs) to update back once solved upstream

[ArmandPhilippot]

Yes, I agree, it's better to have valid docs in the meantime. Reviewed. And while checking the workaround, I noticed we have other issues in https://docs.astro.build/en/guides/sessions/#overriding-the-configuration-at-runtime ... the config (env instead of env.schema 🤦🏽 ) and:

Not sure why yet... but, once this PR is merged, I'll open a follow-up PR to fix that.

---

Personally, I don't mind opening an issue in docs just to keep track of what needs to be done later. An issue doesn't mean this can be fixed immediately, and that seems like the right place for it. And it seems we have a label to communicate that:

So, if you want a note here I'm fine with that! If you prefer another place... not sure where. 😅

[florian-lefebvre]

Yeah sounds good we can keep this one! Thank you

[ArmandPhilippot]

Since the automated PR is stale, and I wanted to fix other things on the Sessions guide, I opened #14482

Regarding tls, I was wrong: this is available. This is the same issue I wanted to investigate related to host on the Sessions guide: I forgot we need extra packages in some cases. And Redis requires to install ioredis. Once the package is installed, both are recognized. But, then, the Sessions guide should include it in the packages to install!