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

fix: remove urlsafe-base64 dependency #218

Merged

Conversation

lenkan
Copy link
Collaborator

@lenkan lenkan commented Feb 15, 2024

In PR #214, I missed that the urlsafe-base64 dependency used the global Buffer object internally. I already had some polyfills added when I tested the change so I missed that we still required the polyfill.

There are a few solutions to this.

  • Re-introduce the polyfill but also set globalThis.Buffer = import("buffer").Buffer.
  • Stop using "Buffer" completely, just use Uint8Array which is available in both browser and node.js runtime.
  • Implement base64url encode/decode using the imported buffer buffer module.

I think the first one is a hack and an anti-pattern to taint global namespace in a library. The second one is a bit too much work for a quick fix. The last one is a good compromise in my opinion, so I did that. Ideally, the buffer module should support base64 url encode/decode but the PR for that has not been merged for some time: feross/buffer#314.

I took some code and test cases from the node.js ecosystem and replaced the urlsafe-base64 dependency with an internal module that uses the buffer package.

Let me know what you think. Can also go with option 1 if you prefer. But a fix is needed to avoid forcing users to polyfill for themselves.

Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 82.86%. Comparing base (e2b2f6e) to head (d534ee3).

Files Patch % Lines
src/keri/core/base64.ts 83.33% 2 Missing ⚠️
src/keri/core/httping.ts 50.00% 1 Missing ⚠️
src/keri/core/indexer.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           development     #218   +/-   ##
============================================
  Coverage        82.86%   82.86%           
============================================
  Files               46       47    +1     
  Lines             4190     4202   +12     
  Branches          1044     1047    +3     
============================================
+ Hits              3472     3482   +10     
- Misses             689      691    +2     
  Partials            29       29           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@psteniusubi
Copy link
Contributor

Less dependencies is better in my opinion. Ultimately I think we should get rid of Buffer completely.

Copy link
Member

@pfeairheller pfeairheller left a comment

Choose a reason for hiding this comment

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

I think this is a good compromise. I doubt there will be any innovations in Base64 encoding/decoding we'll have to keep up with!

@lenkan lenkan force-pushed the fix-base64url-buffer-dependency branch from c6c6bad to d534ee3 Compare March 11, 2024 09:16
@lenkan lenkan merged commit e8b66e9 into WebOfTrust:development Mar 11, 2024
8 checks passed
@lenkan lenkan deleted the fix-base64url-buffer-dependency branch March 11, 2024 14:19
daviddm pushed a commit to daviddm/signify-ts that referenced this pull request May 8, 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.

3 participants