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

[javascript] Remove Buffer usage #1171

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arjunyel
Copy link
Contributor

Motivation

Buffer is Node only and gets converted to a string anyway so user should be responsible to convert based on their platform

Solution

Remove Buffer

@tasn
Copy link
Member

tasn commented Oct 1, 2024

The problem is that this breaks API unfortunately. :(

Is there a real problem with having it be there?

@arjunyel
Copy link
Contributor Author

arjunyel commented Oct 1, 2024

Buffer is a Node only thing, not standard JavaScript so it won't be acceptable in all runtimes.

@tasn would you prefer I close all my PRs and open them against the standard-webhooks library instead? That's much more new and I could do a 2.0 version there with breaking changes that won't effect as many people. In general though the Svix library has node only things which affects people like me who want to use it in other environments like Cloudflare workers, we have to use polyfills

@tasn
Copy link
Member

tasn commented Oct 22, 2024

You don't need a polyfill though, because it's just typing, right? What's the recommended way for typing only kind of thing? Because we do want to support Buffer when that is used.

@arjunyel
Copy link
Contributor Author

Why not have the user be responsible for turning the Buffer into a string? That way it works for every platform. That's the core of the issue, Buffer is a Node API, not a JavaScript one

@tasn
Copy link
Member

tasn commented Oct 22, 2024

Yeah, for sure. The problem is breaking API though. :|

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

Successfully merging this pull request may close these issues.

2 participants