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

Browser compat #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Browser compat #9

wants to merge 3 commits into from

Conversation

vlovich
Copy link

@vlovich vlovich commented Feb 1, 2022

This fixes #7 and #8 by adding support for targeting the browser environment.

Note that iconv-lite is also made optional in case the downstream user doesn't want to take the size hit and is fine only supporting UTF-8 and ASCII. One open question is whether unsupported charsets should be rejected with a RangeError instead of silently failing to decode. Such a change wouldn't be backward compatible but maybe there's a "strict" option
that could be added to decode to control that behavior.

Wrong regexp was used so if iconv is not present then the tests fail.
Don't try to require iconv on browser environments in the first place.
Not sure how to make iconv-lite properly configurable at runtime though.
Remove the need for a polyfill by providing an implementation that uses
web APIs.
@vlovich
Copy link
Author

vlovich commented Feb 1, 2022

This can still hit #7 for Cloudflare Workers from my experiments. Not sure how to do anything other than patching out the iconv-lite require on my end, but I think that's something manageable via patch-package.

@vlovich
Copy link
Author

vlovich commented Feb 2, 2022

Another thing I'll mention is that the primary reason for node-buffer-ops/browser-buffer-ops is to polyfill base 64 with base64-js. Maybe I should inline to use Uint8Array more globally rather than continue to try to use Buffer on node?

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.

Could not resolve "stream"
1 participant