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

Remove connect, disconnect for session strategies #7971

Merged
merged 4 commits into from
Oct 4, 2022

Conversation

emmatown
Copy link
Member

@emmatown emmatown commented Sep 29, 2022

This pull request removes connect and disconnect from the SessionStrategy interface, and replaces the session-store-redis package with an example.

We shouldn't have created a package for what can be effectively reproduced with:

const sessionStrategy = storedSessions({
  store: ({ maxAge }) => ({
    async get(key) {
      let result = await redis.get(key)
      if (typeof result === 'string') {
        return JSON.parse(result)
      }
    },
    async set(key, value) {
      await redis.setEx(key, maxAge, JSON.stringify(value))
    },
    async delete(key) {
      await redis.del(key)
    },
  }),
  // ...
})

export default withAuth(
  config({
    db: {
      // ...
      async onConnect() {
        await redis.connect()
      }
    },
    // ...
  })
)

And an redis.connect() in the db.onConnect hook.

Unfortunately db.onConnect probably should have been called onStartup[but before HTTP].

At the moment, db.onConnect happens to be sequentially when we connect the Prisma client, but strictly has no relationship to if and when the Prisma client is connected or disconnected.
This can lead to DX confusion when you use a context directly from getContext.

@changeset-bot

This comment was marked as resolved.

@vercel

This comment was marked as resolved.

@codesandbox-ci

This comment was marked as resolved.

@vercel vercel bot temporarily deployed to Preview September 29, 2022 07:10 Inactive
@vercel vercel bot temporarily deployed to Preview September 29, 2022 07:24 Inactive
@vercel vercel bot temporarily deployed to Preview September 30, 2022 00:26 Inactive
@dcousens dcousens changed the title Replace session-store-redis-package with example Replace session-store-redis package with example Sep 30, 2022
@emmatown emmatown force-pushed the replace-session-store-redis-pkg-with-example branch from 1730a51 to 76ada52 Compare September 30, 2022 00:51
@vercel vercel bot temporarily deployed to Preview September 30, 2022 00:53 Inactive
@vercel vercel bot temporarily deployed to Preview October 3, 2022 23:11 Inactive
@vercel vercel bot temporarily deployed to Preview October 3, 2022 23:15 Inactive
@emmatown emmatown marked this pull request as ready for review October 4, 2022 00:26
@emmatown emmatown requested a review from a team October 4, 2022 00:26
Copy link
Member

@dcousens dcousens left a comment

Choose a reason for hiding this comment

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

LGTM

@dcousens dcousens merged commit a8a5f1f into main Oct 4, 2022
@dcousens dcousens deleted the replace-session-store-redis-pkg-with-example branch October 4, 2022 01:05
@dcousens dcousens changed the title Replace session-store-redis package with example Remove connect, disconnect for session strategies, replacing session-store-redis package with an example Oct 4, 2022
@dcousens dcousens changed the title Remove connect, disconnect for session strategies, replacing session-store-redis package with an example Remove connect, disconnect for session strategies Oct 4, 2022
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.

2 participants