edgedb / edgedb-js

The official TypeScript/JS client library and query builder for EdgeDB
https://edgedb.com
Apache License 2.0
514 stars 65 forks source link

1078 use right buffer in browser #1079

Closed diksipav closed 2 months ago

diksipav commented 3 months ago

fix #1078

This is tricky, I spend some time today to track this down (@jaclarke helped with some debugging): we need encode/decode for Node, Deno and browser.

We can revert everything to that first solution when checking if global Buffer was available and fallback to atob/btoa versions. In Deno global Buffer should exists so it will be used, in CloudFlare browser replacements will be used. Or investigate further how to use node:buffer in deno and cloudflare by default.

jaclarke commented 3 months ago

I thought we had updated everything to use Uint8Array instead of Buffer so it could work in the browser in this PR: https://github.com/edgedb/edgedb-js/pull/457, so this seems like a regression somewhere?

Edit: The linked issue has the PR with the regression: https://github.com/edgedb/edgedb-js/pull/1015

scotttrinh commented 3 months ago

I thought we had updated everything to use Uint8Array instead of Buffer so it could work in the browser

Yeah, for what it's worth, I think we should try to use the platform's faster/better APIs when we can rather than taking a least-common denominator approach. The issue here was that I did not consider the browser platform when I worked on #1015, only the Deno environment and was leaning on both Deno and Bun having node:buffer compatibility.

We absolutely can just do the base64 decoding ourselves in pure JS, but the old implementation was also subject to unicode problems (atob and btoa don't handle unicode).

At the moment, I think we should attempt to import node:buffer and if we fail, fall back to a pure JS implementation. In the future, I think we should rethink our platform strategy to be able to more easily take advantage of platform-specific APIs and do less runtime/buildtime magic.

Would also be a good idea to add some browser-like smoke tests (maybe JSDom is good enough 🤷 ) to ensure I don't break this again 😅