[ Request ] Replace Buffer with equivalent Uint8Array and crypto with Web Crypto
#74
Conversation
This replaces the use of Buffer with the equivalent method using Uint8Array according to https://github.com/sindresorhus/uint8array-extras#hextouint8arrayhexstring-string-uint8array This makes the package fully serverless compatible.
bitofbreeze
changed the title
|
We'll need to make a corresponding PR to https://github.com/epicweb-dev/totp/blob/main/index.js here to replace crypto with web crypto if possible, so I'll undo some of the doc changes for now. |
|
Hello @bitofbreeze, first of all, thanks for the PR. I usually do not accept changes without a proper use case or an issue you may have had before. If the package works as expected, I usually like to keep the safe path and not introduce any possible new changes, especially if related to security, encryption, etc. This aims to make the library fully serverless, right?
Also, If you are able to merge the same for |
|
It seems like the Vite tests are failing. Not sure if you can check why or if it's related to this PR (I think we were passing those before). |
|
Interesting because it all passes locally for me in bun and node. Will have to figure out what's different. I agree this PR is of little use if a dependency still uses buffer and crypto. I'll make the corresponding PR in epic-web/totp. |
|
Ok the issue I see is a difference between the versions of node. Your test runner uses 16 whereas I am on 21 and indeed it fails locally for me too in 16. So it seems something I'm using is only available in newer versions. But node 16 is end of life so can we update the minimum version to 20? |
|
Yep, that did it @bitofbreeze. Do you know if we will require |
|
We are good to merge without changing epic-web/totp. Just means this package still requires Buffer and node crypto since our dependency uses them. I'm looking at the changes required there and it'll take a little longer than this did--completely doable just more to update. |
|
All right, gonna take the time to review the PR, and if everything looks good to me, this will be merged. Again, thanks for the efforts and the PR, @bitofbreeze! |
|
@dev-xo I made the PR to epic-web/totp epicweb-dev/totp#11 It makes their APIs async, so if they don't want to break it for other consumers, I can repackage it and we can change the dependency here. |
|
Thanks for the efforts @bitofbreeze! I would prefer to keep using Hopefully, Kent accepts the PR, especially if it will improve |
|
Well that was fast! Now I see the responsiveness and helpfulness from Kent that makes you want to stick to @epic-web/totp. I updated the PR to use the new version. |
dev-xo
changed the title
Uint8Array and crypto with Web Crypto
…Web Crypto (#74) This makes the package fully "serverless" compatible. Author: @bitofbreeze
|
Merged @bitofbreeze. Let me know if everything works as expected! Also, feel free to come up with any other suggestions or features you think might be worth considering or having! Again, thanks for the effort and the clean job! |
|
Thanks @dev-xo I removed the extra setup for assigning global Buffer and the nodejs_compat flag from my site and I confirm all works great! You could remove that section of the docs now https://github.com/dev-xo/remix-auth-totp/blob/main/docs/cloudflare.md#vite |
|
Thanks for noticing @bitofbreeze! Kinda busy lately, but I added it to things to look into. Also, thanks for the efforts and the great PR! Feel free to come up with some other improvements you may think could be worth having! |
This replaces the use of Buffer with the equivalent method using Uint8Array according to https://github.com/sindresorhus/uint8array-extras#hextouint8arrayhexstring-string-uint8array and crypto with web crypto.
This makes the package fully serverless compatible without the need for extra steps.