Skip to content
This repository was archived by the owner on Dec 12, 2023. It is now read-only.

fix: allow unstorage drivers other than 'memory' #7

Merged
merged 18 commits into from
Nov 13, 2022

Conversation

Twitch0125
Copy link
Contributor

@Twitch0125 Twitch0125 commented Oct 22, 2022

Closes #8

Contributes to nuxt/nuxt#11734

Fixed by using Nitro's storage layer. The storageOptions were getting stored in the runtime config, but that looks like it does some kind of serializing. I had this config:

session: {
    session: {
      cookieSameSite: 'strict',
      storageOptions: {
        driver: redisDriver({
          base: 'session:',
          url: process.env.REDIS_URL || 'redis://localhost:6379',
        }),
      },
    },
  },

but was getting a " fn is not a function" error.
When I checked the storageOptions in storage.ts, the driver value was getting set to {}.

So now the equivalent config would look like this:

session: {
  session: {
    storageOptions: {
      driver: 'redis',
      url: process.env.REDIS_URL || 'redis://localhost:6379'
    }
  }
}

Also, in storage.ts, Typescript is yelling at me saying useStorage isn't exported from '#imports'. I'm not sure where that actually gets exported if not from there.

Copy link
Contributor

@BracketJohn BracketJohn left a comment

Choose a reason for hiding this comment

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

Hey @Twitch0125 👋

Awesome PR, thanks for the contribution and the clear description ❤️

I want to get this merged, but we need to change some points first. Mainly, this is about allowing the configuration of nuxt-session not to clash with the main-app storage configuration.

The requested changed will also resolve the typescript issues as we stop relying on the useStorage composable of nuxt inside our storage.ts

@BracketJohn
Copy link
Contributor

Btw, I opened nuxt/nuxt#15237 to see if this is a feature or a bug.

@Twitch0125
Copy link
Contributor Author

@BracketJohn Well, I'm stumped.
I'm trying to find a way to provide a useSessionStorage function that works similar to useStorage but with a separate unstorage instance. Initially I was trying to do that with a Nitro plugin, but there's very little documentation on what exactly you can do with those, and how you can add one from a nuxt module.

Looking at the source code for Nitro's useStorage (which I believe is here https://github.com/unjs/nitro/blob/main/src/rollup/plugins/storage.ts), I feel like I'm missing something because surely we don't have to do this whole virtual module thing that nitro is doing, with a nuxt module...

@BracketJohn
Copy link
Contributor

@Twitch0125 I see, then we probably should switch to an approach where we provide: driver-name + options and then use the driver name to instantiate the right storage backend at runtime inside storage.ts?

@Twitch0125
Copy link
Contributor Author

After getting some help from the nuxt discord, I was able to figure this out. nuxt-session now has its own unstorage instance completely separate from nitro's

@BracketJohn
Copy link
Contributor

Yay, this is great - thanks for keeping on pushing!

Can you by resolve the remaining test problems? Right now it's types:
image

Other than that: @valiafetisov will review the next week and then publish once merged (:

@Twitch0125
Copy link
Contributor Author

I was over complicating by creating a virtual module for the session config instead of using useRuntimeConfig.
I ended up adding a @ts-ignore to where I'm using the other virtual module. I tried creating a type for it with declare module '#session-driver' but that didn't seem to be fixing it. But I'm still fairly noobish when it comes to typescript.

src/module.ts Outdated

export type SameSiteOptions = 'lax' | 'strict' | 'none'
export type SupportedSessionApiMethods = 'patch' | 'delete' | 'get' | 'post'

interface StorageOptions {
driver: BuiltinDriverName,
options: object

Choose a reason for hiding this comment

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

Object can be almost everything in javascript, eg array is also an object. Is it possible to specify it further or even extend this type based on the driver name? I assume that every driver has its own set of options, or are they of the same type?

I think something like mapped types might help here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found each of the drivers option types from unstorage, so its using those now.

Co-authored-by: valia fetisov <[email protected]>
@valiafetisov
Copy link

Can you please resolve conflicts? Then its ready to be merged!

@Twitch0125
Copy link
Contributor Author

@valiafetisov resolved!

BracketJohn
BracketJohn previously approved these changes Nov 11, 2022
Copy link
Contributor

@BracketJohn BracketJohn left a comment

Choose a reason for hiding this comment

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

Thanks ❤️ Here one more proposal from me, but please treat it as non-blocking.

Thanks for all the hard work and patience (:

@BracketJohn BracketJohn merged commit 2b4cbb1 into sidebase:main Nov 13, 2022
@BracketJohn
Copy link
Contributor

Thanks @Twitch0125 for the contribution - it was awesome getting this in, I'll do the release shortly.

@BracketJohn
Copy link
Contributor

alright @Twitch0125 the fix is now available at @sidebase/[email protected], see the release notes here: https://github.com/sidebase/nuxt-session/releases/tag/0.2.4

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.

Driver options are not passed through correctly
3 participants