Comments (30)
If you change the check from Buffer.isBuffer(obj)
to obj instanceof Buffer
does it still end up bundling the buffer module in browserify?
from qs.
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);
}
from qs.
I assume you used string keys because you tried testing for global.Buffer
and using global.Buffer.isBuffer(obj);
?
from qs.
Nah, I used the string key because I'm that badass :) Indeed, the non-string approach works as well.
from qs.
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...
from qs.
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]?
from qs.
@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.
from qs.
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...
from qs.
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žė [email protected] 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.
from qs.
@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);
};
from qs.
I love Javascript :)
from qs.
Kind of gross, but seems like it might be the simplest solution in this case.
from qs.
(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
from qs.
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...
from qs.
Closed via #41
from qs.
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.
from qs.
@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...
from qs.
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 :)
from qs.
@dymonaz have you tested with [email protected] or greater? I think this problem should be solved already. Does it not work?
from qs.
@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.
from qs.
Ah, I see. Let me know what you find. I'll reopen this issue for now so I remember it's still not fixed.
from qs.
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?
from qs.
@dymonaz Sure, I don't mind doing that. I'll just need to confirm that there's no effect on performance.
from qs.
@dymonaz I made the buf.constructor
change you requested in feross/buffer@2d92ab6.
It's published as [email protected]
.
from qs.
It's available in browserify immediately, because semver.
from qs.
Thank you, will test and report!
from qs.
All good!
from qs.
Sweet!
On Wed, Oct 29, 2014 at 8:27 AM Dominykas Blyžė [email protected]
wrote:
All good!
—
Reply to this email directly or view it on GitHub
#39 (comment).
from qs.
Awesome! Thanks @dymonaz and @feross for seeing this through to the end. You're awesome!
from qs.
Now published as a standalone module in case others want to use the same trick easily: https://www.npmjs.org/package/is-buffer
from qs.
Related Issues (20)
- Keeping nesting with custom decoder ? HOT 1
- qs.parse decoder option doesn't work on IOS web view environment HOT 1
- Unable to parse back a stringified object when too nested HOT 3
- `stringify` with `arrayFormat: 'comma'` does not encode commas in strings. HOT 13
- [spam]
- ESM & CJS - Built HOT 9
- Stringify null values HOT 9
- None. HOT 1
- Serialization for `Date` is not working when using `filter` option. HOT 3
- Issue using qs while using Express@5 HOT 7
- qs.stringify(json) and qs.parse(json) Results are inconsistent with expectations HOT 4
- Parsing the stringify result would lead to a different object HOT 1
- qs parse is not letting app load on production when imported from index.tsx HOT 1
- Feature request: export another endpoint that doesn't depend on function-bind and possibly other polyfills HOT 10
- Does not work in NextJS edge middleware HOT 3
- Stringify method with format: "RFC1738" does not encode parentheses -> ( ) HOT 3
- I'm appreciate with this library, however my new project use python. Are there any same packages in Python? HOT 1
- How can you keep the square brackets when using arrayFormat: comma HOT 4
- Add required key and type checking HOT 2
- qs.parse return different while the query is in different position HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from qs.