Use native-buffer-browserify

[feross]

Much faster buffer, backed with ArrayBuffer.

• .slice() returns instances of the same type
• Square-bracket buf[4] notation works!
• Does not modify any browser prototypes.
• All tests from the original buffer-browserify project pass.

[ghost]

I approve of this. Just make sure to bump the package major version and we can update the packages that need to be updated downstream.

[feross]

Okay, done!

[AndreasMadsen]

I'm looking at this now :)

[AndreasMadsen]

So this means we no longer support IE8, right. I'm fine with that but its obviously not a decision I can make! I have opened issue #37 and it will need to be resolved first.

Anyway, good job, I haven't executed the complete test suite but it looks good and I think this could solve a lot of issues :)

[feross]

So this means we no longer support IE8

If people care about IE8-9, they can use a polyfill for Uint8Array to get basic Buffer functionality. If they want all the readUint32LE, etc., etc. functions, they need a polyfill for DataView too. There are couple choices of polyfills here: https://github.com/Modernizr/Modernizr/wiki/HTML5-Cross-browser-Polyfills#binary-and-typed-data

Not sure if this should be included by browserify or not.

I think this could solve a lot of issues

Yeah, the whole bops problem for one. Some modules have it, some don't. The ones that have it don't work easily with streams which assume Buffers will be passed in, not Uint8Arrays (which bops returns).

[feross]

Actually, we shouldn't include any polyfills. native-buffer-browserify is smaller than the previous module and adding polyfills would make the total file size larger.

[AndreasMadsen]

Just tested it on chrome (will get around IE later today) and there are 23 failing test (out of 2187). They seams quite important and passes with the current implementation so those bugs should be fixed. I have selected the failing files for you so you can just run:

browserify --transform ./test/browserify-transform.js test/browser/stream2-readable-empty-buffer-no-eof.js test/browser/string-decoder-simple.js | testling -u

to see for yourself.

[AndreasMadsen]

Okay, finished installing my Windows VM :)

In IE11 and I got the same 23 errors. I couldn't test on IE10 since IETester don't support it when I have IE11. I hope this will be fixed soon but lets just assume the result is the same.

[feross]

Thanks for pointing out the failing tests -- I should have caught that. I made some changes and I can confirm that everything passes now.

I ran all the node-browser-builtins tests in the latest stable Chrome, Firefox, Safari, Opera (latest + 12), IE (10 + 11) and they all pass. Additionally, the original buffer-browserify tests (and a few more that I added) also still pass.

[feross]

I confirmed that all browserify tests still pass, with one important caveat: they currently only pass in Node 0.11. The tests use the vm module for test isolation (I think), but vm is buggy in Node 0.10 -- it doesn't make Uint8Array available in the execution context. This appears to be fixed in Node 0.11.

@substack I'm not very familiar with vm. Do you know if there's an easy way to get the Uint8Array constructor into the vm execution context? Maybe we can pass it in?

[AndreasMadsen]

I haven't read the browserify source code but in many cases you can just add the constructor to the sandbox parameter. (But there are also quite a few where you can't).

a = vm.createScript('console.log(Uint8Array([1,1,1]).length)');
a.runInNewContext({ 'Uint8Array': global.Uint8Array, 'console': console }); // prints 3

But since this will be released in a new major version I wouldn't worry that much about the browserify tests.

At last, the pull requests dosen't merge could you rebase?

[feross]

Rebased - try merging now!

[feross]

@AndreasMadsen Neat, adding the constructor to the sandbox parameter worked. Once this PR is merged, I'll send another PR on browserify that bumps the node-browser-builtins version and fixes the tests that were failing.