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

fix: express session type compatibility #190

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

ojeytonwilliams
Copy link
Contributor

Checklist

This PR was motivated by the current need to assert that the Store types are any due to the incompatibility of @fastify/session's types with express-session's. e.g.

server.register(fastifySession, {
  store: new RedisStore({
    client: redisClient,
  }) as any,

I tried to keep the changes to a minimum, but it was necessary to alter the callbacks types and to change precisely what was in the various Session types.

The callback needed updating because express-session's callbacks all define their errors as any and because we can't guarantee that a session is passed to the callback.

The Session types needed updating because the stores are only compatible with express-session's SessionData rather than the full set of properties that a fastify session object has.

I wasn't able to run the benchmarks locally, this PR only modifies types, so I hope that's okay.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit a8773b3 into fastify:master Mar 28, 2023
@ojeytonwilliams ojeytonwilliams deleted the fix/express-session-compat branch April 21, 2023 09:25
@fastify fastify locked as resolved and limited conversation to collaborators Apr 21, 2023
@fastify fastify deleted a comment from fino609 Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants