Is there an alternative to Buffer.isBuffer?

[dominykas]

https://github.com/hapijs/qs/blob/master/lib/utils.js#L134 contains a call to Buffer.isBuffer. This means that when qs@2.x is browserified (we're still on 0.6 for the client side...), the whole Buffer module is pulled in - which adds an extra 40kb (~20kb minified) of code in the bundle - and all of it for just this one function, which is only used once.

I'll try to see if there's a workaround on the browserify side of things, but the question still stands...

The workaround in browserify is to create a fake file which exports Buffer.isBuffer = () => false and expose that as "buffer". It would still be nice to not have to use this workaround...

[nlf]

If you change the check from Buffer.isBuffer(obj) to obj instanceof Buffer does it still end up bundling the buffer module in browserify?

[dominykas]

Just having a typeof Buffer !== 'undefined' injects the whole buffer module. browserify is not trying to be smart about it...

Played around a little, and this seems to fool browserify, but I wonder for how long? Also not sure about correctness across all platforms - but maybe this is worth investigating further?

if ("Buffer" in global) {
    return global["Buffer"].isBuffer(obj);
}

[nlf]

I assume you used string keys because you tried testing for global.Buffer and using global.Buffer.isBuffer(obj); ?

[dominykas]

Nah, I used the string key because I'm that badass :) Indeed, the non-string approach works as well.

[dominykas]

Regarding that commit... Browserify DOES get fooled this way, but we need to keep in mind, that browsers don't define global - window is the global object. I think this tries to solve the problem: https://www.npmjs.org/package/global - but I might be wrong.

Using window.Buffer does fool browserify. We could use this in an IIFE - but it won't work in strict mode: http://perfectionkills.com/unnecessarily-comprehensive-look-into-a-rather-insignificant-issue-of-global-objects-creation/#ecmascript_5_strict_mode

exports.isBuffer = function (obj) {
    return (function () {
        if (typeof this.Buffer !== 'undefined') {
            return this.Buffer.isBuffer(obj);
        }
        else {
            return false;
        }
    }())
};

The above seems to work correctly. I can probably even come up and PR some unit tests for this... Just need to also find an easy way to run them in the browser...

[dominykas]

OK, I think it's worth reverting commit 613b50a and adding "browser": { "buffer": false } in the package.json as per discussion in the browserify thread.

Now, technically, I think, this would be a breaking change? I wonder if anyone at all relies on this behavior [passing Buffer into qs in the browser]?

[latentflip]

@dymonaz if we revert 613b50a, and did "browser": { "buffer": false } I don't think it would be a breaking change would it?

If someone is not using Buffer elsewhere in their browserify application, then there is no change. If they are, then Buffer is not undefined, so there is no change.

I'm pretty sure the only difference then is that we aren't going to optimistically require buffer, which is a win.

I was just about to submit (a different and inferior) fix to this problem, but this one looks better to me, and get's a big 👍 from me.

[dominykas]

If they are, then Buffer is not undefined, so there is no change.

Not exactly. If someone is using Buffer now, then buffer is included in their bundle and browserify replaces the global magically with the one from the buffer module without actually making it global. So, if someone is passing an instance of Buffer into stringify(), it will be correctly detected and toString() will be called.

After the change is made, browserify will not do the magic, therefore Buffer will always be undefined in the qs module, even if it is available inside the bundle, therefore stringify() will not call toString() and probably error out - or even worse - will convert the properties of the actual instance into a query string.

This needs some testing...

[latentflip]

Ah, gotcha, because browserify doesn't make Buffer a global it just adds it to the arg list of the modules function wrapper? Hmm

Philip Roberts

On 26 Sep 2014, at 13:28, Dominykas Blyžė notifications@github.com wrote:

If they are, then Buffer is not undefined, so there is no change.

Not exactly. If someone is using Buffer now, then buffer is included in their bundle and browserify replaces the global magically with the one from the buffer module without actually making it global. So, if someone is passing an instance of Buffer into stringify(), it will be correctly detected. After the change is made, browserify will not do the magic, therefore Buffer will always be undefined in the qs module, even if it is available inside the bundle, therefore stringify() will not call toString() and probably error out - or even worse - will convert the properties of the actual instance into a query string.

This needs some testing...

—
Reply to this email directly or view it on GitHub.

[latentflip]

@nlf how gross would it be to do something like this?

exports.isBuffer = function (obj) {
    return obj.constructor && obj.constructor.isBuffer && obj.constructor.isBuffer(obj);
};

[dominykas]

I love Javascript :)

[nlf]

Kind of gross, but seems like it might be the simplest solution in this case.

[dominykas]

(facepalm) I just ran this as a test with our code... and sure - when there's no Buffer, all is great, but when there is... well... you're not going to like this line: https://github.com/feross/buffer/blob/master/index.js#L58

[dominykas]

OK, so here's the node way of checking for isBuffer: https://github.com/joyent/node/blob/master/lib/util.js#L582
And here's the "browser side" version: https://github.com/feross/buffer/blob/master/index.js#L127

So, the constructor approach just needs to add a check for _isBuffer? And then someone needs to keep tabs on changes in https://github.com/feross/buffer forever...

[nlf]

Closed via #41

[feross]

FYI, buffer is 16KB minified, 5KB gzipped.

$ browserify -r buffer | uglifyjs -c -m | wc -c
16435

$ browserify -r buffer | uglifyjs -c -m | gzip | wc -c
5267

In a large app, it's probably not worth worrying about. For a little module like this one, I totally get the desire to avoid unnecessary dependencies.

[feross]

@dymonaz Regarding the "The Buffer constructor returns instances of Uint8Array that are augmented" comment, I'm open to suggestions to improve this. I'd love it if Buffer was a subclass of Uint8Array instead of just a Uint8Array with instance properties tacked on. Then instanceof Buffer would work correctly, at least sometimes (instanceof doesn't work well with nested module hierarchies). Last I tested, perf was horrible when subclassing Uint8Array. This was the best compromise I could come up with at the time, but maybe things have changed...

[dominykas]

Thanks for pitching in! instanceof Buffer makes browserify auto-include the module - so it's no good.

What would go wrong if you overwrote constructor property? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/constructor thinks it's OK and gives examples, but I guess I need some reading up to do :)

[nlf]

@dymonaz have you tested with qs@2.2.5 or greater? I think this problem should be solved already. Does it not work?

[dominykas]

@nlf I haven't tested post release, but I did try patching it and it didn't seem to work. I plan to revisit tomorrow. It does the right thing on node, but under browserify constructor does not point to Buffer - it points to Uint8Array for performance as @feross mentioned.

[nlf]

Ah, I see. Let me know what you find. I'll reopen this issue for now so I remember it's still not fixed.

[dominykas]

I can confirm - on 2.3.1, after removing all the hacks I had in place, the buffer is not included in my bundle by default, just because qs is included - so that part is OK. But if I actually pass an instance of Buffer on the browser side, qs will fail to detect it as such on browsers that have support for typed arrays (detects correctly on IE10).

@feross would you care to set buf.constructor = Buffer after https://github.com/feross/buffer/blob/master/index.js#L94?

[feross]

@dymonaz Sure, I don't mind doing that. I'll just need to confirm that there's no effect on performance.

[feross]

@dymonaz I made the buf.constructor change you requested in feross/buffer@2d92ab6.

It's published as buffer@2.8.0.

[feross]

It's available in browserify immediately, because semver.

[dominykas]

Thank you, will test and report!

[dominykas]

All good!

[feross]

Sweet!
On Wed, Oct 29, 2014 at 8:27 AM Dominykas Blyžė notifications@github.com
wrote:

All good!

—
Reply to this email directly or view it on GitHub
#39 (comment).

[nlf]

Awesome! Thanks @dymonaz and @feross for seeing this through to the end. You're awesome!

[feross]

Now published as a standalone module in case others want to use the same trick easily: https://www.npmjs.org/package/is-buffer