Improve browser compatibility for Buffer usage #29

jstarry · 2021-07-08T15:12:56Z

Fixes: #28

Changes

Explicitly require buffer module so that bundlers can polyfill it for browser apps but use the global Buffer if available.
Loosened type checks from Buffer.isBuffer to instanceof Uint8Array to improve API for browser apps

jstarry · 2021-07-08T15:19:19Z

@pabigot this is ready for review, thanks!

pabigot

OK, this isn't quite what I thought. Rather than replace the entry API use of Buffer with Uint8Array, it replaces the Node.js Buffer with a reimplementation.

I see this as being useful for browser applications, but a step backwards for Node.js applications. The performance comparison suggests BrowserBuffer can be up to 4x slower than native Node.js (new, slice, and wrie operations particularly seem affected).

Can you come up with a solution that continues to use the native Buffer implementation in Node.js environments, but adds the external implementation only in browser environments?

If not, can you explain exactly why people who use this in Node.js won't be upset at the new unnecessary dependency and potential conflict of having multiple Buffer implementations in their applications?

If not, then does it make more sense for you to fork this project and publish your changes as browser-buffer-layout? That'd also simplify ongoing maintenance here, as the only changes I've had to make in the last three years are to support an environment that wasn't a target for the original solution.

test/LayoutTest.js

lib/Layout.js

package.json

.travis.yml

pabigot · 2021-07-09T14:16:11Z

lib/Layout.js

@@ -2354,9 +2354,11 @@ class Blob extends Layout {
    if ((offset + span) > b.length) {
      throw new RangeError('encoding overruns Uint8Array');
    }
-    uint8ArrayToBuffer(b).write(src.toString('hex'), offset, span, 'hex');
+    const buffer = uint8ArrayToBuffer(b);


In the earlier commit you use:

b = uint8ArrayToBuffer(b);

I'd rather see consistency so please change this to match. Also srcBuffer should just change the variable src.

Also: why is this necessary? The test seems to pass at the commit before this; if this is necessary there should be a new test case that fails without this change to demonstrate what's going on.

Depending on what motivated this change, it might be squashed into the one that changes all the other uses, so there bisecting wouldn't hit a commit that has a bug that resulted from the change in underlying implementation.

Motivation of this change is that if the API supports Uint8Array params, we need to polyfill the Buffer.write method which doesn't exist on Uint8Array.

Added a new test case that fails without the conversion for b and src

jstarry · 2021-07-09T19:39:28Z

lib/Layout.js

+  // Node v4 doesn't respect byteOffset and length arguments
+  if (buffer.length > b.length) {
+    buffer = buffer.slice(b.byteOffset, b.byteOffset + b.length);
+  }


Without bumping the min node version, we need a little polyfill to account for the fact that node v4 doesn't have support for the byteOffset and length arguments to the Buffer constructor.

As noted in the review summary, I'm going to remove v4 support so you won't need this.

jstarry · 2021-07-09T21:41:06Z

@pabigot I've taken your feedback into account and this PR is now ready for another pass, thanks!

pabigot

This is getting close, thanks.

Overall I feel your change warrants a minor release update to 1.3.0. Because of that, I've also updated various things related to packages and integrations that hadn't been touched in the last three years. Those are now in the pu branch, which is queued to merge to the next branch. Please take a look at #31, and comment on whether you have concerns. After you've taken a look, or by the middle of next week, I'll merge that to the next branch.

Once those changes are in next I'd like you to retarget your PR to next instead of main. Since #31 removes the support for Node 4 and provides a rationale for doing so, one of your polyfills won't be needed anymore. Also I'd like you to remove release-related changes from your PR: the version number in package.json, and the description in CHANGELOG.md. I'll handle those when I do the new release. There will be one trivial conflict to resolve in lib/Layout.js when you rebase.

Also, please update all commit messages to the format:

   tag: short description

   details of change...

   Signed-off-by: yourname

The sentence/paragraph(s) in the body should describe the change and explain why it's being made, so somebody looking at changes in a commit can understand what they were intended to do. For example for 4cd65bf it should be something like:

    Layout: Select Buffer implementation based on environment

    Use the presence of a variable defined in browser environments to
    select whether the Buffer implementation is built in to the
    environment or needs to be retrieved from a (potentially external)
    module.  This allows use of the package in Node.js, pure
    ECMAScript, and browser environments.

    Signed-off-by: Justin Starry <justin@solana.com>

ffb8f3a should explain why Uint8Array is preferred over Buffer, using text from #28. And so on.

A good summary of the policy I've used is here or here. The commits in #31 can also serve as an example.

pabigot · 2021-07-10T11:40:20Z

test/LayoutTest.js

@@ -1761,6 +1761,24 @@ suite('Layout', function() {
      assert.equal(bl.span, 3);
      assert.equal(bl.property, 'bl');
    });
+    test('uint8array', function() {


Name this one basics, since the effect of this patch series is to change the base packed data representation from Buffer to Uint8Array. Then rename the old basics to Buffer to prove Buffer arguments still work.

CHANGELOG.md

pabigot · 2021-07-10T18:45:33Z

lib/Layout.js

+  // Node v4 doesn't respect byteOffset and length arguments
+  if (buffer.length > b.length) {
+    buffer = buffer.slice(b.byteOffset, b.byteOffset + b.length);
+  }


As noted in the review summary, I'm going to remove v4 support so you won't need this.

coveralls · 2021-07-10T19:24:34Z

Coverage remained the same at 100.0% when pulling 0ccc868 on jstarry:uint8array into 93dfbe0 on pabigot:next.

jstarry · 2021-07-10T19:25:25Z

@pabigot I've prepped this branch so that it should be ready for merge once pu is merged. Really appreciate your feedback and diligence in maintainership!

Use the presence of a variable defined in browser environments to select whether the Buffer implementation is built in to the environment or needs to be retrieved from a (potentially external) module. This allows use of the package in Node.js, pure ECMAScript, and browser environments. Signed-off-by: Justin Starry <justin@solana.com>

Loosen Buffer type checks to accept the Uint8Array base class. Uint8Array is preferred over Buffer due to its presence in both Node.js and browser environments. When Buffer methods are needed, create a new Buffer instance which uses the same underyling memory of the Uint8Array parameter. Signed-off-by: Justin Starry <justin@solana.com>

jstarry · 2021-07-10T20:39:54Z

lib/Layout.js

+/* istanbul ignore if */
+if ('undefined' !== typeof window && 'undefined' !== typeof window.Buffer) {


Ignored here, let me know if you disagree with taking this route

pabigot · 2021-07-10T22:30:52Z

This looks fine. Please confirm that this branch works as intended in a browser environment, since there's currently nothing in place to confirm that.

jstarry · 2021-07-10T23:05:03Z

Great, it shouldn't be too bad to add a test for that with mocha. I added an issue to track adding browser test coverage: #32

jstarry · 2021-07-10T23:05:34Z

Thanks again @pabigot, feel free to merge at your leisure, I don't have permission.

pabigot · 2021-07-10T23:09:30Z

This is a big change; I don't want to publish a version that claims to work in a browser without confirmation it works in that environment. Can you install it in the environment you need it in and confirm that this specific code (not just your original version) works there? I agree automated testing is highly desirable, but I don't even know how to do it manually.

jstarry · 2021-07-10T23:40:11Z

Sure thing, I have two branches here: https://github.com/jstarry/buffer-layout-test/branches. The success branch uses my changes and the failure branch uses the released v1.2.2 package.

buffer-layout has a couple of browser-specific improvements in pabigot/buffer-layout#29 and pabigot/buffer-layout#27 that this update pulls in

jstarry changed the title ~~Improve browser compatibility by loosening Buffer type to Uint8Array~~ Improve browser compatibility for Buffer usage Jul 8, 2021

pabigot requested changes Jul 9, 2021

View reviewed changes

jstarry force-pushed the uint8array branch 4 times, most recently from d2ad9d0 to 7cf00df Compare July 9, 2021 19:37

jstarry commented Jul 9, 2021

View reviewed changes

jstarry force-pushed the uint8array branch 4 times, most recently from 38f417e to 6fb0246 Compare July 9, 2021 21:39

pabigot requested changes Jul 10, 2021

View reviewed changes

jstarry changed the base branch from main to pu July 10, 2021 18:55

jstarry changed the base branch from pu to next July 10, 2021 18:56

jstarry force-pushed the uint8array branch from 6fb0246 to 73ba6cc Compare July 10, 2021 19:23

jstarry force-pushed the uint8array branch 2 times, most recently from 20cef21 to 02e4a75 Compare July 10, 2021 20:30

jstarry added 2 commits July 10, 2021 15:39

jstarry force-pushed the uint8array branch from 4d5c683 to 0ccc868 Compare July 10, 2021 20:39

jstarry commented Jul 10, 2021

View reviewed changes

pabigot approved these changes Jul 10, 2021

View reviewed changes

pabigot merged commit 0ccc868 into pabigot:next Jul 11, 2021

jeduan added a commit to jeduan/anchor that referenced this pull request Dec 6, 2021

Update buffer-layout

4e76e89

buffer-layout has a couple of browser-specific improvements in pabigot/buffer-layout#29 and pabigot/buffer-layout#27 that this update pulls in

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve browser compatibility for Buffer usage #29

Improve browser compatibility for Buffer usage #29

jstarry commented Jul 8, 2021 •

edited

Loading

jstarry commented Jul 8, 2021

pabigot left a comment

pabigot Jul 9, 2021

jstarry Jul 9, 2021

jstarry Jul 9, 2021

jstarry Jul 9, 2021

pabigot Jul 10, 2021

jstarry commented Jul 9, 2021

pabigot left a comment •

edited

Loading

pabigot Jul 10, 2021

pabigot Jul 10, 2021

coveralls commented Jul 10, 2021 •

edited

Loading

jstarry commented Jul 10, 2021

jstarry Jul 10, 2021

pabigot commented Jul 10, 2021

jstarry commented Jul 10, 2021

jstarry commented Jul 10, 2021

pabigot commented Jul 10, 2021

jstarry commented Jul 10, 2021 •

edited

Loading

		/* istanbul ignore if */
		if ('undefined' !== typeof window && 'undefined' !== typeof window.Buffer) {

Improve browser compatibility for Buffer usage #29

Improve browser compatibility for Buffer usage #29

Conversation

jstarry commented Jul 8, 2021 • edited Loading

Changes

jstarry commented Jul 8, 2021

pabigot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry commented Jul 9, 2021

pabigot left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 10, 2021 • edited Loading

jstarry commented Jul 10, 2021

Choose a reason for hiding this comment

pabigot commented Jul 10, 2021

jstarry commented Jul 10, 2021

jstarry commented Jul 10, 2021

pabigot commented Jul 10, 2021

jstarry commented Jul 10, 2021 • edited Loading

jstarry commented Jul 8, 2021 •

edited

Loading

pabigot left a comment •

edited

Loading

coveralls commented Jul 10, 2021 •

edited

Loading

jstarry commented Jul 10, 2021 •

edited

Loading