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

perf: replace jsonwebtoken with jose #8217

Merged

Conversation

andershermansen
Copy link
Contributor

The jose package has 0 dependencies and is tree shakable ESM.
So we get lower bundle size and get rid of 10 dependencies.

The jose package has 0 dependencies and is tree shakable ESM.
So we get lower actual bundle size and get rid of 10 dependencies.
@andershermansen
Copy link
Contributor Author

@AlessioGr On the quest for lower bundle size this will reduce payload bundle size from 4,008 KB to 3,824 KB (4.6% reduction).

In addition there are some security concerns with the lodash packages included in jsonwebtoken. Ref the comment here: auth0/node-jsonwebtoken#933 (comment)

All in all jose seems more modern, updated and is ESM. I have used it successfully in other projects instead of jsonwebtoken.

I have also checked that the generated token with my change is same as before using the debugger on https://jwt.io/

@r1tsuu r1tsuu requested a review from AlessioGr September 14, 2024 19:47
@AlessioGr AlessioGr self-assigned this Sep 14, 2024
@jmikrut
Copy link
Member

jmikrut commented Sep 15, 2024

Hey @andershermansen good PR! We will review shortly. I think we will be able to merge it.

THANK YOU!!!

@andershermansen
Copy link
Contributor Author

@jmikrut @AlessioGr Any concerns about the change? Or just still waiting for code review?

@AlessioGr
Copy link
Member

No concerns atm, just need to find time to review

@DanRibbens
Copy link
Contributor

I reviewed this PR. Code wise it all seems fine.

I don't see a lot of difference between the two packages though: https://bundlephobia.com/package/[email protected] https://bundlephobia.com/package/[email protected].

@andershermansen
Copy link
Contributor Author

@DanRibbens The payload package will still be reduced by 4,6% if this is included if my calculations are correct.

I have calculated it like this:
After pnpm install and pnpm of this commit (10032b5):
npx esbuild --platform=node --bundle src/index.ts --outfile=out_jose.js --analyze > log_jose.txt 2>&1

and the last commit before it (6d1a287):
npx esbuild --platform=node --bundle src/index.ts --outfile=out_jsonwebtoken.js --analyze > log_jsonwebtoken.txt 2>&1

File sizes:

$ du -k out_j*
1928	out_jose.js
2020	out_jsonwebtoken.js

That is a 92kB reduction in size. (92 / 2020 = 4,6%)
I'm not sure if this way of calculating the size is relevant with how you build and distribute payload?

Jose is fully ESM and tree shakable while jsonwebtoken is not.

You also get rid of 10 dependencies, some of which are discouraged and will not be updated by loadash in newer versions. See https://lodash.com/per-method-packages

Copy link
Member

@AlessioGr AlessioGr left a comment

Choose a reason for hiding this comment

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

I like this. jsonwebtoken is a major contributor to our amount of dependencies, thus this change is very welcome

CleanShot 2024-10-18 at 00 27 48@2x

@AlessioGr AlessioGr merged commit ef8a5b1 into payloadcms:beta Oct 18, 2024
48 checks passed
@andershermansen andershermansen deleted the replace-jsonwebtoken-with-jose branch October 18, 2024 16:31
Copy link
Contributor

🚀 This is included in version v3.0.0-beta.117

@emonadeo
Copy link

emonadeo commented Oct 23, 2024

Can we get the jwtSign method exported for library use?

For context I am using a custom passkeys authentication and I would prefer not to duplicate internal payload code since it might break in between updates.

@sharpsteelsoftware
Copy link

I would also love to see jwtSign exported, or better yet, payload Local API to support an operation for verifyToken()

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.

6 participants