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

Unify local and refresh provider #821

Closed
2 of 4 tasks
zoey-kaiser opened this issue Jul 19, 2024 · 2 comments · Fixed by #822
Closed
2 of 4 tasks

Unify local and refresh provider #821

zoey-kaiser opened this issue Jul 19, 2024 · 2 comments · Fixed by #822
Labels
1.x enhancement An improvement that needs to be added provider-local An issue with the local provider provider-refresh An issue with the refresh provider

Comments

@zoey-kaiser
Copy link
Member

zoey-kaiser commented Jul 19, 2024

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

  • AuthJS
  • Local
  • Refresh
  • New Provider
@zoey-kaiser zoey-kaiser added enhancement An improvement that needs to be added pending An issue waiting for triage labels Jul 19, 2024
@zoey-kaiser
Copy link
Member Author

zoey-kaiser commented Jul 19, 2024

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

  • Option 1: Just by checking if endpoints.refresh is set and then enabling the refresh logic
  • Option 2: Adding config value called enableRefresh (or something similar), which then executes the refresh regardless of whether the endpoint is set (throws an error if it tries to refresh and no endpoint is set).

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

  • refresh(): Method to refresh using the refreshToken
  • refreshToken: Access the current refreshToken

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:

  • refresh(): Throw an error that no refresh endpoint is set
  • refreshToken: Always is undefined

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

@phoenix-ru
Copy link
Collaborator

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):

export interface CommonUseAuthReturn<SignIn, SignOut, GetSession, SessionData> {
data: Readonly<WrappedSessionData<SessionData>>;
lastRefreshedAt: Readonly<Ref<SessionLastRefreshedAt>>;
status: ComputedRef<SessionStatus>;
signIn: SignIn;
signOut: SignOut;
getSession: GetSession;
refresh(): Promise<unknown>
}

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)

@zoey-kaiser zoey-kaiser added provider-local An issue with the local provider provider-refresh An issue with the refresh provider and removed pending An issue waiting for triage labels Aug 11, 2024
phoenix-ru added a commit that referenced this issue Aug 22, 2024
* updated types to match new refresh / local provider logic

* removed playground for refresh provider, began updating useAuthState logic for local to handle refresh tokens

* finished merging local and refresh providers

* removed tests for refresh playground

* change the tsconfig to inherit the local playground tsconfig

* fix: types

* Apply suggestions from code review

* fix: remove refresh test ci

* fix: improve signOut body composition

* added remark about refeshToken being null if refresh.isEnabled is false

* Also added jsdoc for internal UseAuthReturn type

* Added types for refresh cookie httpOnly flag and improved JSDocs for it

* updated outdated doc links

* improve jsdocs on refreshOnlyToken

* fix: build

* chore: minor adjustments

* removed left over package.json

* enh: use json pointer for signout request

---------

Co-authored-by: Marsel Shayhin <18054980+phoenix-ru@users.noreply.github.com>
Co-authored-by: Marsel Shaikhin <phoenix.apps@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x enhancement An improvement that needs to be added provider-local An issue with the local provider provider-refresh An issue with the refresh provider
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants