Type safety: `Context.state` is unsound

[connec]

I'm exploring fresh 2 via deno run -Ar jsr:@fresh/init.

In the example app, State and define are exported from utils.ts as follows:

import { createDefine } from "fresh";

// This specifies the type of "ctx.state" which is used to share
// data among middlewares, layouts and routes.
export interface State {
  shared: string;
}

export const define = createDefine<State>();

My first instinct on reading this was that something must be required to set a default value for State when constructing the app, otherwise there's no way to ensure it will match its type.

Looking inside main.ts we find:

import { App, staticFiles } from "fresh";
import { define, type State } from "./utils.ts";

export const app = new App<State>();

app.use(staticFiles());

// Pass a shared value from a middleware
app.use(async (ctx) => {
  ctx.state.shared = "hello";
  return await ctx.next();
});

// <snip>

So, we're able to instantiate an application without passing a default State value. The default middleware described as "pass a shared value" is in fact doing the job of initialising the state, without it the state's value is simply {}. If ctx.state.shared = "hello" is commented out, TypeScript remains happy while the runtime value of the field is undefined.

Looking into the code, the culprit is here:

fresh/packages/fresh/src/context.ts

Lines 81 to 82 in a1000cb

	/** State object that is shared with all middlewares. */
	readonly state: State = {} as State;

This is unsound unless {} is actually assignable to State (e.g. all State's fields are optional), the as just makes TypeScript ignore the issue.

Depending on the project's view on type safety this might be acceptable, e.g. it's up to the framework user to either supply a state type for which {} is a valid value, or to ensure that nothing depends on it before it's properly initialised (as is the case for the init project).

Otherwise, I can imagine three alternatives that would be sound:

1. Require initialisation, e.g. new Context(...) should also take a value for state. This could be integrated by, e.g., new App(...) having an extra argument defaultState: State which is copied into new Contexts (or defaultState: () => State which is called for each request).

2. Make the state type Partial<State>, e.g. Context.state: Partial<State> = {}. This is type safe (no cast needed) but forces all uses of the state to deal with the possibility of fields being uninitialised, even if the application logic guarantees they would be initialised by that point.

3. Make the uninitialised state explicit, e.g. Context.state: State | {} = {} or Context.state: State | "uninitialized" = "uninitialized". As above, this is type safe but forces all uses to deal with uninitialisation.

To me, the best option would seem to be the first. That would allow framework users to provide a complete default if possible, and if not possible they can account for it in whichever way they prefer:

• They can achieve the current behaviour by using {} as State as the default.
• They can use an initialisation scheme that makes sense for each field of the state (e.g. default value, optional fields, or explicit uninitialised states).

[connec]

I've realised this can be handled conveniently outside the framework by making the State type partial, e.g.

// This specifies the type of "ctx.state" which is used to share
// data among middlewares, layouts and routes. It must be `Partial`,
// since Fresh assigns `{}` as a default value.
export type State = Partial<{
  shared: string;
}>;

I guess this corresponds to users ensuring {} is assignable to their state type.

[marvinhagemeister]

Yeah it's one of the oldest issues with TypeScript type system that it cannot where type safety for a standard compose function.

The current approach tries to find a pragmatic middle ground where you can use the State interface to prevent errors apart from initializing it. If you have an idea to improve in this I'm all ears. It would be awesome if this can be made fully type safe.

[marvinhagemeister]

The approach chosen by kito with their .extend() helper might be a good solution to this: https://kito.pages.dev/docs/#extensible