-
Notifications
You must be signed in to change notification settings - Fork 26
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
Replace jsonwebtoken with jose #276
Conversation
Seems like a pragmatic approach even though it would be good to get to the bottom of why so many fail to run with jsonwebtoken@9 (I suspect polyfills for Feel free to ditch node 14 from the matrix, and then release as a major release. |
113b429
to
5629213
Compare
@trotzig I noticed that the tests failed on node 14 due to jose using some syntax that is not supported until node 16. To address this I decided to stop running the tests on node 14 (and added 20 and 22 while I was at it). What do you think about that? update: I just saw your comment about this. 🐣 |
Looks like node 20 and 22 tests aren't working out (https://github.com/happo/happo.io/actions/runs/9571942826/job/26390148668?pr=276), so I'll be looking to add those to the test matrix in a follow up. I think one of the error messages we are asserting against is a little different for some reason. |
It looks like we just have to update the assertion? |
We received a report of this package failing with the following error: > TypeError: Right-hand side of 'instanceof' is not an object The stack trace pointed at the jsonwebtoken package as the source. At first it looked like this may be a problem on older versions of Node, but it turned out that they were using a relatively recent version (v18.17.1). It appears that a lot of other people may be running into a similar issue: - auth0/node-jsonwebtoken#939 - auth0/node-jsonwebtoken#892 People have reported success by either downgrading jsonwebtoken to v8 or replacing it with jose. Since we updated to jsonwebtoken v9 to fix a security vulnerability, I don't think it is a great idea to go back to v8. jose seems to be well-supported, healthy, and active: https://snyk.io/advisor/npm-package/jose Let's give this a shot! The API is a little bit different than jsonwebtoken, but for the most part I think the new version is simple enough to transition to. I set the alg properity to HS256, since it is required to specify the alg and that is what jsonwebtoken uses as the default. It seems that jose uses `||=` syntax which is not supported on node 14, so I am also removing the node 14 tests as part of this commit.
In #276 we replaced the jsonwebtoken package with jose, which uses syntax that does not work on node 14 or earlier. I think it might be worthwhile to make this compatibility a little more explicit in our engines field.
In #276 we replaced the jsonwebtoken package with jose, which uses syntax that does not work on node 14 or earlier. I think it might be worthwhile to make this compatibility a little more explicit in our engines field.
We received a report of this package failing with the following error:
The stack trace pointed at the jsonwebtoken package as the source.
At first it looked like this may be a problem on older versions of Node, but it turned out that they were using a relatively recent version (v18.17.1).
It appears that a lot of other people may be running into a similar issue:
People have reported success by either downgrading jsonwebtoken to v8 or replacing it with jose. Since we updated to jsonwebtoken v9 to fix a security vulnerability, I don't think it is a great idea to go back to v8.
jose seems to be well-supported, healthy, and active:
https://snyk.io/advisor/npm-package/jose
Let's give this a shot!
The API is a little bit different than jsonwebtoken, but for the most part I think the new version is simple enough to transition to. I set the alg properity to HS256, since it is required to specify the alg and that is what jsonwebtoken uses as the default.
It seems that jose uses
||=
syntax which is not supported on node 14, so I am also removing the node 14 tests as part of this commit. I'll look into adding node 20 and 22 to the test matrix in a followup.