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

Add Buffer compatibility for browsers #765

Closed
arboleya opened this issue Feb 6, 2023 · 14 comments
Closed

Add Buffer compatibility for browsers #765

arboleya opened this issue Feb 6, 2023 · 14 comments
Assignees
Labels
bug Issue is a bug

Comments

@arboleya
Copy link
Member

arboleya commented Feb 6, 2023

We need to support Buffer usage inside the Browsers.

For example, someone using React with Vite might fall into this error:

error

And then having to resort to custom mapping configurations such by installing the buffer package:

pnpm install buffer

And then configuring custom mappings in vite.config.js:

import { defineConfig } from 'vite'
import react from '@vitejs/plugin-react'

// https://vitejs.dev/config/
export default defineConfig({
  plugins: [react()],
  resolve: { alias: { buffer: 'node_modules/buffer/index.d.ts' } },
})
@arboleya arboleya added the bug Issue is a bug label Feb 6, 2023
@arboleya
Copy link
Member Author

arboleya commented Feb 6, 2023

This might relate to the following PR, in the sense that the Buffer usages should fail for the browser group:

cc @Dhaiwat10

@Dhaiwat10
Copy link
Member

@arboleya none of our browser tests are failing in #728. I am using puppeteer to run the tests in a browser. Will need to investigate the difference between that environment and Vite. Do you have a repo containing a reproducible example that I can run locally to see this Vite error?

@arboleya
Copy link
Member Author

arboleya commented Feb 7, 2023

@Dhaiwat10 I created this issue from a conversation with @Braqzen.

@Braqzen Do you happen to have a simple repro for this problem?

@Braqzen
Copy link

Braqzen commented Feb 7, 2023

@Dhaiwat10 I created this issue from a conversation with @Braqzen.

@Braqzen Do you happen to have a simple repro for this problem?

The steps are something like this (hopefully a repro):

  1. pnpm create vite@latest ui --template react-ts
  2. mkdir /ui/src/contract-typegen
  3. cd /ui
  4. pnpm install
  5. pnpm install fuels --save
  6. Generate contract typegen files with the fuels package by pointing at the generated /out/debug/-abi.json and placing them in the /ui/src/contract-typegen directory

The contract that I have is a simple counter. It has a single storage variable which is incremented and decremented by 1 when the function is called. It has a couple more functions but I doubt they matter.
I think you need to import the wallet from fuels and potentially have some code to call a function from the contract in a .tsx file in order for it to trigger.
Try starting the server and opening it in the browser at this point and I believe the error should pop up in the console of the browser. If not then I can create a local repository and link it here.

@camsjams
Copy link
Contributor

camsjams commented Feb 9, 2023

This can appear within any browser environment that is using Cryptography IIRC, but most typical setups include a polyfill that makes Buffer exist.

We wouldn't want to add anything into TS-SDK to change how that works, but we should at least document and test for it.

@Dhaiwat10
Copy link
Member

Yeah, Vite is one of the few 'popular' setups that don't polyfill these things. I believe that's the reason why none of our tests are failing in #728 when run in a browser environment - it uses puppeteer which runs a headless version of Chromium under the hood.

@arboleya
Copy link
Member Author

arboleya commented Feb 9, 2023

@Braqzen In which browser have you had this problem?

@camsjams Why do you think we should not add buffer as a dependency? I wonder if Polyfills should be required only for libraries there were not originally planned to run inside the Browser.

@Braqzen
Copy link

Braqzen commented Feb 9, 2023

@Braqzen In which browser have you had this problem?

@camsjams Why do you think we should not add buffer as a dependency? I wonder if Polyfills should be required only for libraries there were not originally planned to run inside the Browser.

I was running things in Chromium.

@camsjams
Copy link
Contributor

camsjams commented Feb 9, 2023

@arboleya Why do you think we should not add buffer as a dependency? I wonder if Polyfills should be required only for libraries there were not originally planned to run inside the Browser.

Mostly due to the fact that it's environment specific (browser), dependent on build tool, and easier for consumer to add then remove.

If it's solely just inside the dependencies then that's fine with me

@arboleya
Copy link
Member Author

arboleya commented Jun 5, 2023

@danielbate
Copy link
Contributor

danielbate commented Sep 7, 2023

We will need to polyfill Buffer (along with other Node environment deps) should we want to configure a complete browser testing environment, akin to the one that Vite offers. Unlike the browser-like environment that puppeteer and playwright offer. Also relates to #284.

@Torres-ssf Torres-ssf assigned Torres-ssf and danielbate and unassigned Torres-ssf Dec 8, 2023
@arboleya arboleya added this to the Beetle milestone Dec 14, 2023
@arboleya arboleya modified the milestones: 2 - Beetle, 1 - Salamander Jan 8, 2024
@danielbate
Copy link
Contributor

So for browser testing in #1630, I went down the consumer configuration route, rather than introducing the polyfill our side. Purely from the stance of trying to introduce minimal interruption to the packages themselves, and only changing the test infrastructure. On doing some digging, this is what's recommended rather than attempting to account for it on our side.

I did have success in polyfilling it our side via tsup configuration with an esbuild injection if we want to go down that route.

I could also do an example with documentation based on the vite configuration that we have used for browser testing. However I don't think I will go any further in #1630 given we now have a setup with minimal interruption.

@arboleya arboleya modified the milestones: 1 - Salamander, 2 - Beetle Jan 15, 2024
@arboleya arboleya modified the milestones: 2 - Beetle, 4 - Cricket Jan 31, 2024
@danielbate danielbate assigned nedsalk and unassigned danielbate Jun 7, 2024
@arboleya arboleya added the p2 label Jun 9, 2024
@arboleya arboleya added this to the 0.x post-launch milestone Jun 12, 2024
@nedsalk nedsalk removed their assignment Jul 17, 2024
@arboleya arboleya removed this from the 0.x post-launch milestone Jul 19, 2024
@arboleya
Copy link
Member Author

@FuelLabs/sdk-ts Pretty old one - is this still valid?

@arboleya arboleya removed the p2 label Jul 20, 2024
@danielbate
Copy link
Contributor

@nedsalk and I chatted around the browser testing time and ended up opting to polyfill at the app level, as we do with the browser tests. This discussion sums it up well.

Going to close this for now as we've gone back and fourth quite a bit, it could be revisited if raised by users but it's not a blocker as an app dev can resolve it.

@danielbate danielbate closed this as not planned Won't fix, can't repro, duplicate, stale Jul 22, 2024
@danielbate danielbate self-assigned this Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants