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

Change to replace Buffer with Uint8Array #85

Merged
merged 1 commit into from
Jun 10, 2023
Merged

Change to replace Buffer with Uint8Array #85

merged 1 commit into from
Jun 10, 2023

Conversation

wooorm
Copy link
Member

@wooorm wooorm commented Jun 9, 2023

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Previously, this project depended on Node’s buffer. This commit changes that: it replaces that support with Uint8Array. In most cases, this will be fine, because the Node Buffer class subclasses Uint8Array.
However, there are differences in which encodings are supported when turning this binary data into a string, and there are differences in how methods work.

Previously, this project depended on Node’s buffer.
This commit changes that: it replaces that support with `Uint8Array`.
In most cases, this will be fine, because the Node `Buffer` class
subclasses `Uint8Array`.
However, there are differences in which encodings are supported
when turning this binary data into a string, and there are differences in
how methods work.
@wooorm wooorm requested a review from remcohaszing June 9, 2023 14:18
@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually labels Jun 9, 2023
@github-actions github-actions bot removed the 👋 phase/new Post is being triaged automatically label Jun 9, 2023
@ChristianMurphy
Copy link
Member

However, there are differences in which encodings are supported when turning this binary data into a string, and there are differences in how methods work.

What encodings are no longer supported?
Hopefully UTF-8 is still supported?

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

I take it this would be SemVer major?
Are there changes which will be needed in unified?

new VFile(Buffer.from('bar')).toString(),
'bar',
'buffer: should return the internal value'
new VFile(Buffer.from([0xef, 0xbb, 0xbf, 0x61, 0x62, 0x63])).toString(),
Copy link
Member

Choose a reason for hiding this comment

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

does this mean buffer from string is no longer supported, and only hex can be used?

@wooorm
Copy link
Member Author

wooorm commented Jun 9, 2023

This is indeed semver major!

What encodings are no longer supported?

depends on how chrome/node/etc is built, see also https://github.com/vfile/vfile/pull/85/files#diff-92bbac9a308cd5fcf9db165841f2d90ce981baddcb2b1e26cfff170929af3bd1R614, so typically everything is supported, but some Node-isms like hex was tested for and is not supported here

does this mean buffer from string is no longer supported, and only hex can be used?

nope, those are fine! This just updated the 2 tests to be more clear by using bytes and actually different encodings, instead of already unicode strings

So it’s mostly going to be fine!

@wooorm wooorm merged commit f4edd0d into main Jun 10, 2023
23 checks passed
@wooorm wooorm deleted the drop-buf branch June 10, 2023 07:59
@wooorm wooorm added 🧑 semver/major This is a change 💪 phase/solved Post is done labels Jun 10, 2023
@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Jun 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done 🧑 semver/major This is a change
Development

Successfully merging this pull request may close these issues.

None yet

2 participants