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

feat: add refreshTokenHandler option #7237

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

WojtekTheWebDev
Copy link
Collaborator

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSDoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Adds a new optional property refreshTokenHandler to the middlewareModule

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@WojtekTheWebDev WojtekTheWebDev requested a review from a team as a code owner August 21, 2024 13:30
Copy link

changeset-bot bot commented Aug 21, 2024

πŸ¦‹ Changeset detected

Latest commit: 6810360

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@vue-storefront/sdk Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Collaborator

@bartoszherba bartoszherba left a comment

Choose a reason for hiding this comment

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

Few comments qdded

[ADDED] new option to the `middlewareModule` - `refreshTokenHandler`.
This special handler can be used to handle 401 errors and refresh the token.
It is called before the generic `errorHandler`.
By default, it thrown an error which is being caught by the `errorHandler` and rethrown.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that some small diagram (even ascii) with the sequence will be useful here. Or maybe we can somehow rephrase this sentence, I think I would not understand it without my knowledge about how it works. Something like an explanation that by default we are not able to predict the mechanism of token refresh because of differences in different services therefore we do not provide any default behavior, we simply push the error to the errorHandler without any further actions.

methodName,
url,
params,
config,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a module config? Or what kind of config? Will I have some docs in TS? If not maybe lets explain the injected object here?

@WojtekTheWebDev WojtekTheWebDev marked this pull request as draft August 22, 2024 19:47
@WojtekTheWebDev
Copy link
Collaborator Author

Converted to draft as I'm still not convinced that it is the best solution.

Copy link

sonarcloud bot commented Oct 28, 2024

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