signOut / getSessionId should not be part of helpers

[ycmjason]

Hello,

Thanks for the great work!!

oauthConfig is not referenced at all in signOut and getSessionId (see here).

They work are "provider-agnostic" so they shouldn't be bound under a provider config.

I would like to suggest them to be moved back out from Helpers. This way when implementing user auth with multiple providers, the sign out / get session id flow wouldn't be confusing.

I am happy to pick this up if the author agrees with the vision.

Jason

[ycmjason]

But obviously they depend on options, so perhaps options should be set from a different level instead of at createHelpers. Perhaps a feasible api would be something like...

const kvOAuth = createKVOAuth(options)

const { signIn, handleCallback } = kvOAuth.createSignInHelpers(createOAutuConfig())

kvOAuth.signOut(request)

kvOAuth.getSessionId(request)

[iuioiua]

The helpers all depend on the options object. This achieves consistent configuration between all the helpers. Moving signOut() and getSessionId() out of the helpers would make setup less ergonomic for the user and make sharing the shared state a little more cumbersome. We won't be implementing this. Either way, thank you for the suggestion.

[ycmjason]

@iuioiua

Thank you for the response. I believe my earlier comment addresses the concern about consistent configuration. The options object should be a per-app configuration because it determines where the session ID is stored, and it's fair to assume one session per user. Pulling options a level up makes sense, as shown in my earlier suggestion:

const kvOAuth = createKVOAuth(options)
const { signIn, handleCallback } = kvOAuth.createSignInHelpers(createOAuthConfig())
kvOAuth.signOut(request)
kvOAuth.getSessionId(request)

This also ensures better consistency, with different providers sharing the same options and avoiding issues like different cookie names for each provider. Would love to hear your thoughts.

[ycmjason]

As a follow-up, it may be helpful to look at prior art like Firebase Auth SDK. Firebase treats the configuration as global for the app, allowing multiple providers to share session management and configuration. This ensures consistency across providers. Here's a rough example:

const auth = firebase.auth();
const googleProvider = new firebase.auth.GoogleAuthProvider();
const facebookProvider = new firebase.auth.FacebookAuthProvider();

auth.signInWithPopup(googleProvider);
auth.signInWithPopup(facebookProvider);

auth.signOut();
auth.currentUser;

This approach aligns with what I’m suggesting, ensuring consistent session handling and avoiding provider-specific configurations.

[sylc]

Moving signOut() and getSessionId() out of the helpers would make setup less ergonomic for the user

I also recently started to use this library for multiple providers and was confused by this problem as well. I had to read the source code to realised that i can use any signOut or getSessionId, but it is confusing. The current api seems less ergonomic to me.

example below in code of my original confusion

const oauthConfig = createGitHubOAuthConfig();
const githubHelpers = createHelpers(oauthConfig);

const googleOauthConfig = createGoogleOAuthConfig();
const googleHelpers = createHelpers(oauthConfig);

async function handler(request: Request) {
  const { pathname } = new URL(request.url);
  switch (pathname) {
   ...
    case "/oauth/signout":
      return await <WHICH_HELPER_DO_I_USE_HERE_?>.signOut(request);  
    case "/protected-route":
      return await <WHICH_HELPER_DO_I_USE_HERE_?>.getSessionId(request) === undefined
        ? new Response("Unauthorized", { status: 401 })
        : new Response("You are allowed");
    default:
      return new Response(null, { status: 404 });
  }
}

As a compromise the 2 way could work together i.e helpers could still export, signOut() and getSessionId() but these function could also be exported from the main module. (Although my preference would also be to remove them the helpers for clarity)

@iuioiua could you reconsider ? thanks

[ycmjason]

@iuioiua in case you miss the above.

[iuioiua]

Sorry for the delayed message. Are you able to submit a PR with your suggested changes?

[ycmjason]

I am more than happy to pick this up.

Before I do so tho, perhaps it's worth exploring how we want the API to look like? Here are a few options I can think of atm.

Option 1: createKvOAuth

Move signOut and getSessionId to a level up by exposing a createKvOAuth function.

interface CreateKvOAuthOption {
  cookieOptions: Partial<Cookie>;
}

declare const options: CreateKvOAuthOption;
const kvOAuth = createKvOAuth(options)

declare const config1: OAuth2ClientConfig;
const helpers1 = kvOAuth.createSignInHelpers(config1)

declare const config2: OAuth2ClientConfig;
const helpers2 = kvOAuth.createSignInHelpers(config2)

helpers1.signIn(request)
heleprs1.handleCallback(request)
helpers2.signIn(request)
heleprs2.handleCallback(request)
kvOAuth.signOut(request)
kvOAuth.getSessionId(request)

Option 2: pass in config everytime

interface CreateKvOAuthOption {
  cookieOptions: Partial<Cookie>;
}

declare const options: CreateKvOAuthOption;
const kvOAuth = createKvOAuth(options)

declare const config1: OAuth2ClientConfig;
declare const config2: OAuth2ClientConfig;

kvOAuth.signIn(request, config1)
kvOAuth.handleCallback(request, config1)

kvOAuth.signIn(request, config2)
kvOAuth.handleCallback(request, config2)

kvOAuth.signOut(request)
kvOAuth.getSessionId(request)

Happy to hear more ideas from the community.