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

Open to use conditional exports instead of globalThis.Buffer? #87

Closed
ukstv opened this issue Feb 6, 2024 · 10 comments
Closed

Open to use conditional exports instead of globalThis.Buffer? #87

ukstv opened this issue Feb 6, 2024 · 10 comments

Comments

@ukstv
Copy link
Contributor

ukstv commented Feb 6, 2024

Hey, @achingbrain recently it was discovered that some crypto wallets forcefully polypill globalThis.Buffer with some garbage Buffer implementations. uint8arrays uses globalThis.Buffer quite extensively. I guess, the intent is to use native Node.js Buffer, not some polyfill. How do you think about moving Buffer-related code behind conditional exports?

Say, you could have #alloc which resolves to ./buffer-alloc.js when node-imported, and ./uint8array-alloc.js by default.

Also, node.js Buffer could be found in node:buffer on Bun and Node.js, which makes the code a bit simpler to read, and it definitely avoids implications of globalThis pollution.

@achingbrain
Copy link
Owner

it was discovered that some crypto wallets forcefully polypill globalThis.Buffer with some garbage Buffer implementations

For my own education do you have some links for this?

Yes, though, I am open to doing this in a better way. A few points:

  1. It uses globalThis.Buffer instead of importing from node:buffer to avoid needing extra config for bundlers
  2. Conditional exports work when consuming this module but they don't override relative imports from within the module

It might just need the node:buffer stuff putting in it's own file and a "browser" field override in the package.json file for it after all.

@ukstv
Copy link
Contributor Author

ukstv commented Feb 12, 2024

For my own education do you have some links for this?

Plug wallet for ICP https://plugwallet.ooo injects globalThis.Buffer Apparently, it is quite popular. Has 100,000 users according to Google Chrome Web Store: https://chromewebstore.google.com/detail/plug/cfbfdhimifdmdehjmkdobpcjfefblkjm

Yes, though, I am open to doing this in a better way

Whatever you prefer!

Conditional exports work when consuming this module but they don't override relative imports from within the module

My bad, was not precise enough. I thought about Subpath Imports, which seem to be supported all right by bundlers.

@ukstv
Copy link
Contributor Author

ukstv commented Feb 12, 2024

To confirm on your own:

  1. Install Plug wallet on Chrome.
  2. Open dev console on any page
  3. Type globalThis.Buffer => apparently, it is there.

@ukstv
Copy link
Contributor Author

ukstv commented Feb 19, 2024

@achingbrain Any news? Are you open to help from my end?

@achingbrain
Copy link
Owner

Hey, yes - please open a PR with the proposed changes and we can discuss there.

@ukstv
Copy link
Contributor Author

ukstv commented Feb 19, 2024

Great!

@ukstv
Copy link
Contributor Author

ukstv commented Feb 26, 2024

@achingbrain So, I made a possible solution for this issue: #88 Please take a look and say what you think about it.

@ukstv
Copy link
Contributor Author

ukstv commented Feb 26, 2024

@oed Just FYI, so that you are in the loop here.

@ukstv
Copy link
Contributor Author

ukstv commented Mar 6, 2024

@achingbrain Please, could you have a look at the PR for moving out of globalThis.Buffer?

@ukstv
Copy link
Contributor Author

ukstv commented Mar 13, 2024

Yahoo!!!

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

No branches or pull requests

2 participants