-
Notifications
You must be signed in to change notification settings - Fork 30
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!: provide fully decoded jwt object to key callback #335
feat!: provide fully decoded jwt object to key callback #335
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This is a breaking change for anybody retrieving keys asynchronously, so it needs a major semver if we go ahead with this. Is there a way to introduce this without a breaking change?
- We need tests to cover this change, I appreciate this doesn't seem to have been covered well in the past since it doesn't require any changes to tests (apart from naming)
- Does it need typings changes?
Yep, this would need a major version. I don't think we can escape doing a breaking change here unless we do as suggested on the issue an put this behind a configuration flag but at that point I would think it's less work to update the code to the next version than to add a new config option for that since the user would only have to change a couple of characters in their code.
I'll add some tests to cover this.
Yes! I missed the types file in this repo. I will add those too. |
@simoneb I have updated the types and wrote a new test that covers this change. About updating consumers the of this lib, I searched for |
@guilhermelimak this package is used by @fastify/jwt, which in turn is used by fastify-auth0-verify, and potentially others. Let's assess the impact of this change there too. Although @fastify/jwt is not in our organization, it's still something we actively contribute to, so let's consider it as part of our ecosystem. |
@simoneb After using this search I wasn't able to find any other places to change this besides the one I already mentioned. Looks to me like this feature is not widely used by fastify-jwt or it's consumers. there are a couple that are using a callback on the key but they don't use any of the arguments so they shouldn't be affected. If you have any more ideas for other searches I could do I can look more into it, if not I think it's a pretty safe change to make. |
@guilhermelimak ok thanks for checking. One last thing to check before we go ahead with this. If the consumers we know about and which are packages are exposing the options provided by this plugin directly to their consumers, we need to bump semver majors of those as well. Can you write down the list of those plugins? |
@simoneb We can merge this PR and create the release, then we can open a PR on this repo which is directly affected by our changes and bump fastify-jwt which affect it's consumers indirectly. I didn't find other repositories that we manage that are affected so after that we should be done. |
I'll do the first step @guilhermelimak , you then take care of the rest please. Note that fastify-jwt is not ours, so let's make sure we communicate clearly |
This PR enables receiving the whole JWT object for the key callback instead of only the headers.
Close #330.