xmtp / xmtp-js

XMTP client SDKs for JavaScript applications.
https://xmtp.org/docs
210 stars 38 forks source link

[DRAFT] Add isomorphic Buffer #652

Open killthebuddh4 opened 3 weeks ago

killthebuddh4 commented 3 weeks ago

This change obviates the Buffer polyfill workaround using the "@xmtp/rollup-plugin-resolve-extensions" package.

I've confirmed all tests (except for one already-failing test) still pass with yarn test. I've also smoke-tested the change with a "vanilla" (yarn create vite) Vite-based React project. I also plan to test with create-react-app, create-next-app, and any other tools that make sense.

IMO this change would be a big win for users, does anyone have any thoughts? Should I go ahead and test the other environments?

changeset-bot[bot] commented 3 weeks ago

⚠️ No Changeset found

Latest commit: 22674497b9e931e422bdc0b565bd129e81e4670b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

killthebuddh4 commented 2 weeks ago

Any thoughts on this @neekolas ?

rygine commented 2 weeks ago

hey @killthebuddh4, thanks so much for taking the time to contribute!

we made the deliberate decision to not include a Buffer polyfill automatically. this allows developers to choose how they polyfill and which polyfill they want to use.

did you run into any specific issues when polyfilling?

killthebuddh4 commented 1 week ago

hey @killthebuddh4, thanks so much for taking the time to contribute!

we made the deliberate decision to not include a Buffer polyfill automatically. this allows developers to choose how they polyfill and which polyfill they want to use.

did you run into any specific issues when polyfilling?

It's my pleasure to contribute! I'm a big believer in XMTP's utility and am working on some libraries/apps that leverage it.

I'm not having any specific issues at the moment because I've implemented the workaround enough times that I know what to do when I run into it. That said, I guess I believe that having to implement a workaround at all is the specific issue. My perspective (at least in the context of this PR) is less a user's perspective and more the perspective of someone who wants more developers to use XMTP. The polyfill is a pretty rough edge when it comes to usability.

Here's some of my thinking:

@xmtp/xmtp-js depends quite strongly on Buffer. I think it makes sense that @xmtp/xmtp-js would manage that dependency like any other dependency. It seems like a not great user experience to ask the user to manage the dependency, especially since the ways to do it are all very not ideal. I'm trying to think of reasons why a user would have opinions about which version of Buffer @xmtp/xmtp-js should be using, but I'm drawing a blank.

I kind of want to stress that (IMO) the polyfill workaround is a really not fun user experience. I think the typical JS developer tries very hard to avoid dipping into the shadowy world of JS build tooling 😬. An ergonomic JS lib should probably require zero, or as close to zero as possible, config to get up and running.

The polyfill workaround is going to get ran into by basically everybody when they first install @xmtp/xmtp-js and those developers will have to search for the solution, and it won't be completely obvious to everybody where they should look for the answer. FWIW the solution is not actually in the docs anymore (or at least the page linked to in the README doesn't have the answer).

This changeset mirrors (exactly, I think) the way @xmtp/xmtp-js handles the crypto API, so this would be maybe a pretty stress-free, low-risk change? I'm not 100% sure, but I think it'd be just a patch version bump?

So the tl;dr is basically that IMO the workaround is a pretty big usability loss without a corresponding flexibility win. What do you think?

rygine commented 1 day ago

The polyfill is a pretty rough edge when it comes to usability.

I agree that it's not ideal, but it's something that most web devs run into at some point, so the concept hopefully isn't entirely foreign.

FWIW the solution is not actually in the docs anymore

Thanks for pointing this out! We recently changed our docs framework and some of the content moved around. I've updated the README to make it very clear that a Buffer polyfill is required for browser environments and linked to the correct docs with a solution.

I'm trying to think of reasons why a user would have opinions about which version of Buffer @xmtp/xmtp-js should be using, but I'm drawing a blank.

Most folks will likely use buffer, but we don't want to make those kinds of decisions for developers when there's a clear path to polyfill.

All of this being said, when V3 is fully operational, Buffer will not be needed.