sidebase / nuxt-auth

Authentication built for Nuxt 3! Easily add authentication via OAuth providers, credentials or Email Magic URLs!
https://auth.sidebase.io
MIT License
1.25k stars 162 forks source link

Unify `local` and `refresh` provider #821

Closed zoey-kaiser closed 1 month ago

zoey-kaiser commented 2 months ago

Describe the feature

The current local and refresh providers share many of the same logic, aside from the refresh provider having an additional endpoint that lets you refresh the session.

Maintaining both providers separately, although they share a lot of the same logic has lead to some inconsistencies between the two. With the release of 1.0.0 we plan to unify the two providers, enabling the additional logic of the refresh provider by checking if endpoint.refresh was set. If it was, then the logic of the refresh provider is enabled, otherwise the local provider remains as it currently is.

Provider

zoey-kaiser commented 2 months ago

@phoenix-ru, I am currently debating how to detect if the refresh endpoint should be called. I can see two ways to handle this:

In addition, we need to decide how to handle the missing returns of useAuth. Currently, the refresh provider also returns:

This method and value will also be typed for local setups without a refresh system when unifying the two providers. My recommendations for the two properties would be as follows:

This feels like the most "native" solution and would properly reflect to developers why the two are not working.

phoenix-ru commented 2 months ago

I would suggest a simpler solution (Option 2 in your case): have a flag useRefresh: true (enableRefresh also sounds good to me!) on provider config. Maybe we can do some TS-magic to force endpoints.refresh to be set when this flag is toggled. Or just doing a runtime check on module initialization is good already. Thinking about a flag, I'd move all refresh logic into a new field on local provider, something like:

{
  type: 'local',

  refresh: {
    isEnabled: true,
    onlyToken: true,
    token: { }
  }
}

What do you think? Of course, feel free to come up with better namings/organization


Regarding refresh(), we already have such a method on all providers (I recently introduced it to solve some pains regarding refreshing a session): https://github.com/sidebase/nuxt-auth/blob/429b61504affa5ada75e679c7e552ba13ef9dd65/src/runtime/types.ts#L482-L490

refreshToken: Always is undefined

I think this is fine.


In summary: I can see both advantages and disadvantages of merging the providers. The biggest disadvantage to me so far is that we have a (imho) great TS setup to separate the two and provide type hints. The quality of hints will get lower and we would need to add refresh-logic only badge to docs (which we pretty much do anyways)