1078 use right buffer in browser #1079
Conversation
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 We absolutely can just do the base64 decoding ourselves in pure JS, but the old implementation was also subject to unicode problems ( At the moment, I think we should attempt to import 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 😅 |
|
|
||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const Buffer = require("node:buffer").Buffer; |
There was a problem hiding this comment.
The tests are failing because we cannot use require in Deno, and Deno will unconditionally try to run this code. We typically can get around this by putting the non-Deno stuff in the adapter.node.ts module, and making a Deno-compatible import available in adapter.deno.ts, and at buildDeno-time, we swap these out in the source. This is what we did before by exporting Buffer from the adapter.
I think we should be able to do that if we move these base64-related functions into the adapter.* modules and import them instead of importing Buffer. And having the adapter.node.ts module do this check instead.
There was a problem hiding this comment.
this will probably not work in browser because we will import from adapter.node and that file imports node libs that are not available in browser
There was a problem hiding this comment.
yeah, that's true. I didn't really consider how this module before was totally runtime-agnostic, where the adapter stuff was always imported into server-only contexts.
#1003 the "Buffer not defined" err was thrown because on line 35 typeof was missing: if (Buffer === "function") {. So native node Buffer was never used until new PR that fixes that but broke browser functioning.
Let's just adopt this where we fix the check, and keep most of the old code. I think a more thorough fix will come when we make platform-specific packages so the modules don't need to be so dynamic, but we are still a ways off from working on that.
There was a problem hiding this comment.
@scotttrinh @jaclarke I think this should be it. Btw I didnt add any new tests, can do that too.
| let encodeB64: (_: Uint8Array) => string; | ||
|
|
||
| // @ts-ignore: Buffer is not defined in Deno | ||
| if (typeof Buffer === "function") { |
There was a problem hiding this comment.
Can you try this form and see if it allows us to remove the @ts-ignore here?
| if (typeof Buffer === "function") { | |
| if (typeof globalThis.Buffer === "function") { |
A recent update of pnpm (9.8.0) started to cause failures. I believe this is due to the "temporary" projects being made within the main repo's directory, causing some weird "project" detection in pnpm.
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.
Here the "Buffer not defined" err was thrown because on line 35
typeofwas missing:if (Buffer === "function") {. So native node Buffer was never used until new PR that fixes that but broke browser functioning.Require doesn't work in Deno. Test is failing. I believe err should be caught in catch block but for some reason test fails.
In browser require can work but requiring
node:Buffernot. Node prefix is not recognised by Webpack, so building is failing. So I guess going with this try/catch block with require in try block will anyway not be good enough.CloudFlare needs nodejs compat flag if node comp libs are used. I don't know what we want here to be default behaviour for Cloudflare, do we use browser libs by default or we use node compat libs and require users to set Cloudflare to use node libs.
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.