fixes for <=ie8

[ghost]

I want to start using this module directly from browserify when people require('querystring') but will need non-es5 support, which this patch enails.

[defunctzombie]

I disagree with this patch. We should either use modules for these or not include these shims.

[Gozala]

@substack @defunctzombie how about adding dependencies on instead ?

https://npmjs.org/package/lodash.isarray https://npmjs.org/package/lodash.keys https://npmjs.org/package/lodash.map

[Gozala]

Also @medikoo has being doing some maintenance work on this so I'd rather make him decide.

[defunctzombie]

I really wish those lodash modules had real repos and tests and not this whole roundabout show; otherwise using those seems reasonable if you want legacy support out of the box.

[Gozala]

@substack I would really love if I could specify some optional dependencies for packages that are only included for specific bundles. So I could specify that this module depends on es5-shims for ie6, ie7, builds.

[defunctzombie]

The way to do that is at the app level and with global shims. Since you know the users browser. On Dec 3, 2013 4:12 PM, "Irakli Gozalishvili" notifications@github.com wrote:

@substack https://github.com/substack I would really love if I could specify some optional dependencies for packages that are only included for specific bundles. So I could specify that this module depends on es5-shims for ie6, ie7, builds.

— Reply to this email directly or view it on GitHubhttps://github.com/Gozala/querystring/pull/5#issuecomment-29751843 .

[Gozala]

The way to do that is at the app level and with global shims. Since you know the users browser.

That implies app developer has to digg into all it's dependencies to find out weather they use es5 features or not. I would rather let packages tell that forthemselfs and just decide browser support level at bulid time browserify --ie6 ...

[defunctzombie]

It doesn't. Just assume for old browsers you need shims. No need to over engineer this dying problem. The amount of byes for shims is tiny and will only be needed for some of your users. Hell, other parts of your app will probably need way more tweaking than shipping a shims :) On Dec 3, 2013 4:24 PM, "Irakli Gozalishvili" notifications@github.com wrote:

The way to do that is at the app level and with global shims. Since you know the users browser.

That implies app developer has to digg into all it's dependencies to find out weather they use es5 features or not. I would rather let packages tell that forthemselfs and just decide browser support level at bulid time browserify --ie6 ...

— Reply to this email directly or view it on GitHubhttps://github.com/Gozala/querystring/pull/5#issuecomment-29752879 .

[medikoo]

@Gozala thanks for honor. I'm humbled :)

I don't want to sound as a bad guy here, but for me it's clear No.

It's 2013, most of widely used browsers stand on ES5. The only one, IE8, can easily be fixed by referencing following in a head <!--[if IE]><script src="es5-shim.js"></script><![endif]-->.

There are also developers (like me), who do not build for ES3 engines anymore, and for them fix as proposed becomes an obsolete baggage that can't be opt out. I totally wouldn't like to see that tens of ES5 based packages I use import ES3 fixes to the browser on their own, not to mention each of them having shims inline or each of them importing different shim. It looks more as modularity anti-pattern to me.

I'd say not having ES5 support is a special rare case here that should be addressed outside and not be part of a logic of this package.

---

If we're after ES3 support and IE only cc fix is for some reason not perfect, I think the best way to solve is as you suggested in comments. CJS bundler like Browserify should just parse modules for ES5 usage and preload specific shims on his own. This is the feature I have on my todo list for Webmake.

[ghost]

This is fine if you don't want this patch, but I'm going to fork this module anyways in that case because it is absolutely necessary for browserify v3 and I don't want to wait around asking for permission.

[ghost]

The remaining exclusion of this patch is a roadblock for browserify v4.

[medikoo]

This module works perfectly and resembles as it should node implementation.

ES3 compatibility is Browserify requirement and not npm, so such thing should be handled within Browserify. I would suggest to re-open this issue in Browserify issue tracker.

[ghost]

It is the responsibility of individual modules to work in the browser, not browserify.

[defunctzombie]

Your polyfills don't match what is on MDN (especially for isArray). I think it would break in some cases where the real polyfill would not. This module does work in the browser, just not in all the browsers that some other project wants it to work in :)

[medikoo]

@substack this package is mirror of Node's querystring package, and same as Node.js version it is based on ECMAScript5 and works flawlessly in all environments that implement it, including latest versions of all major browsers. I use it in a browser myself.

I understand that one of the goals of Browserify (project you maintain) is to provide ECMAScript3 based versions of modules provided natively in Node. In that case I suggest you create ES3 version of each package, e.g. querystring-es3 and use that instead. It's much more generic approach, and better than wandering around other modules and asking them to downgrade because your project needs that.

Alternatively you can also prepare transform or plugin for browserify, which will detect usage of all shimable ECMAScript5 functions, and provide such shims on demand. This is how I plan like to solve that issue in Webmake.

[mike-spainhower]

Can we revisit this patch? Reasonable philosophical arguments aside, it is only 25 added lines in a project that appears to stay pretty static (removing the maintenance considerations from the equation).

Also, the fork https://www.npmjs.com/package/querystring-es3 appears to get ~2x downloads/mo compared to this module https://www.npmjs.com/package/querystring . This is of course all from browserify, but I think the point is that this PR isn't from someone doing a weekend hackathon competition, it supports a huge number of serious developers running code in production.

I may just be smacking the hornet's nest and I apologize if that is the case, but I implore you to give this change another consideration.

@medikoo @Gozala

[medikoo]

@mike-spainhower all has been said. It's not package made specifically for Browserify. It is environment agnostic and bundler agnostic one. If Browserify decided to be also ES5 to ES3 transpiler, it should solve that problem on own ground, and not demand all outer packages to adjust to that.