feat: replace Buffer-based base64 paths with engine-neutral paths#98
feat: replace Buffer-based base64 paths with engine-neutral paths#98Phillip9587 wants to merge 1 commit into
Conversation
Migrate base64 encode/decode paths away from Node.js-specific Buffer to @exodus/bytes for cross-engine compatibility. Add base64 benchmarks for Buffer, @exodus/bytes, base64-js, and Uint8Array.fromBase64()/toBase64() with TextEncoder/TextDecoder, using runtime guards.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #98 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 55 61 +6
Branches 20 22 +2
=========================================
+ Hits 55 61 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Phillip9587 Using Makes me feel like it should still be |
|
We may also want to consider applying |
|
@blakeembrey Hey, I took a look at your refactoring of my PR in #95. Please give me some time to explain my reasoning behind this PR versus #95. It’s currently holiday in Germany, so I’m away for the weekend. |
|
Of course, no rush! In case it helps, my context is that it's a trade-off between package size and performance. Since |
|
I ran the same comparison locally, and I agree that on Node, It's not just about performace it is whether we want
For We are also only using the two focused exports we need, I hope this explains why I prefer |
|
The easy solution here is to allow encode/decode to be passed as an option, and document it in the README. The dependency makes the package both slower and larger. From 529 B to 3.43 kB (almost 7x). I understand the concerns around maintaining multiple code paths, but I'm not convinced it's worth the trade off. What consistency and formatting are you concerned about across runtimes? Since this packages primary consumer is node.js, and presumably any non-node runtime will be a more up to date, having only two supported paths ( Another thing that's possible with semver is to adopt this later if it's requested, while it's not going to be possible to remove it without a major. I'd personally opt for the smaller and faster package, and upon request from users, evaluate expanding scope. |
|
I'd prefer using only |
|
Makes sense. I’m good with both directions. The consistency concern I had was mainly at the codec boundary: strict UTF-8 vs loose with replacement characters, permissive base64 handling, and avoiding subtle differences between runtime APIs. But I agree that this is probably better handled as an escape hatch instead of pulling in a dependency by default. |
Migrate base64 encode/decode paths away from Node.js-specific Buffer to
@exodus/bytesfor cross-engine compatibility. Add base64 benchmarks forBuffer,@exodus/bytes,base64-js, andUint8Array.fromBase64()/toBase64()withTextEncoder/TextDecoder, using runtime guards.https://github.com/ExodusOSS/bytes does the heavy lifting of choosing the right implementation per environment.
Performance Comparison accross engines:
https://docs.google.com/spreadsheets/d/1GnQYzrzEdF3Ea1hJUoEkEZEVNUI3ve8PKPFhLWcEoBI/edit?gid=0
https://github.com/ExodusOSS/bytes/blob/main/Performance.md
I decided to use the strict utf8 methods that throws on invalid UTF-8 byte sequences. We can also use the loose methods that uses the replacement char instead of throwing but I think the strict method is more correct.