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

Remove Buffer usage for Browser Environments #423

Merged
merged 4 commits into from
Feb 5, 2024

Conversation

curran
Copy link
Contributor

@curran curran commented Jan 28, 2024

Closes #418

@curran curran marked this pull request as ready for review January 28, 2024 16:13
@curran curran mentioned this pull request Jan 28, 2024
test/printer/index.ts Outdated Show resolved Hide resolved
test/printer/index.ts Outdated Show resolved Hide resolved
@Conduitry
Copy link
Member

Are there concerns with atob/btoa's handling of non-ascii characters?

@curran
Copy link
Contributor Author

curran commented Jan 29, 2024

That would be great to add test cases for.

For example

const str = "Hello, world! 😊 Привет, мир! こんにちは世界";

Here's one way that could be solved.

export const stringToBase64 =
    typeof Buffer !== 'undefined'
        ? (str: string) => Buffer.from(str).toString('base64')
        : (str: string) => btoa(new TextEncoder().encode(str).reduce((acc, byte) => acc + String.fromCharCode(byte), ''));

export const base64ToString =
    typeof Buffer !== 'undefined'
        ? (str: string) => Buffer.from(str, 'base64').toString()
        : (str: string) => new TextDecoder().decode(Uint8Array.from(atob(str), c => c.charCodeAt(0)));

In order to actually test that code path, the tests would need to run in a browser environment.

test/printer/index.ts Outdated Show resolved Hide resolved
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
@curran curran changed the title Remove buffer usage Remove buffer usage for Browser Environments Jan 29, 2024
@curran curran changed the title Remove buffer usage for Browser Environments Remove Buffer usage for Browser Environments Jan 29, 2024
@curran curran changed the title Remove Buffer usage for Browser Environments Remove Buffer usage for Browser Environments Jan 29, 2024
@curran
Copy link
Contributor Author

curran commented Jan 29, 2024

I added the handling of non-ASCII characters. I haven't tested this yet in an actual browser environment. I hope to do that soon.

@curran
Copy link
Contributor Author

curran commented Jan 29, 2024

It works!

Before

image

After

image

I tested it by merging the browser build branch and this one into a local build, then used npm link to include it in my local build of https://github.com/vizhub-core/vzcode , which thanks to this new work will shortly have Prettier support for Svelte files!

Thanks all for the reviews and comments. This is the magic of open source right here.

@curran
Copy link
Contributor Author

curran commented Jan 31, 2024

I've addressed all the feedback and I believe this is ready to go. Thanks!

@curran
Copy link
Contributor Author

curran commented Jan 31, 2024

Some alternative implementations:

function base64Encode(str) {
  const encoder = new TextEncoder();
  const encoded = encoder.encode(str);
  return btoa(String.fromCharCode(...encoded));
}

function base64Decode(str) {
  const decoder = new TextDecoder();
  const bytes = Uint8Array.from(atob(str), c => c.charCodeAt(0));
  return decoder.decode(bytes);
}

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@dummdidumm dummdidumm merged commit 99c885e into sveltejs:master Feb 5, 2024
1 check passed
@curran
Copy link
Contributor Author

curran commented Feb 5, 2024

My pleasure! Thanks for reviewing and merging!

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.

Remove dependency on Buffer
3 participants