stringify: Avoid arr = arr.concat(...), push to the existing instance

[papandreou]

Currently qs.stringify will create a new array whenever a new parameter is added. This fixes it by pushing to the existing one instead.

Very unscientific benchmark:

time node -e "const qs = require('./');for (let i = 0 ; i < 1000000 ; i += 1)qs.stringify({a: 1, b: 2, c: 3, d: 4, e: 5, f: 6, g: 7, h: 8, i: 9, j: 10, k: 11, l: 12, m: 13, n: 14, o: 15, p: 16, q: 17, r: 18, s: 19, t: 20, u: 21, v: 22, x: 23, y: 24, z: 25});"

Before:

real	0m11.247s
user	0m11.260s
sys	0m0.093s

After:

real	0m6.291s
user	0m6.305s
sys	0m0.052s

[ljharb]

I'm hesitant to do this; 5 seconds for a million iterations means that in the common case, it will be a scattering of milliseconds' difference.

[ljharb]

this changes a dependency on concat to a dependency on both push and apply.

Separately, concat actually has different semantics - it uses isConcatSpreadable, which includes more than just arrays (altho perhaps that doesn't matter here).

[papandreou]

Why is it bad to depend on Array#push and apply? They're fairly standard :). It'd look nicer with array spread, of course.

I had no idea about Symbol.isConcatSpreadable. As far as I can tell, the operand will always be an array or a string, though.

[ljharb]

They're all standard, but the more things that the package depends on existing, the more likely it will break if people mess with the builtin prototypes.

[papandreou]

That's technically correct, but is it really worth worrying about? It seems unlikely that anyone will overwrite ES3 methods from prototypes in this day and age? Especially methods that have been in the language for 20 years?

I'm caught a bit off-guard because I haven't heard that kind of reasoning for preferring one piece of code over another for a very long time. I'm aware of MooTools, should.js and some of the even crazier things people did before, but hasn't the community stopped doing it and moved on at this point?

[ljharb]

This is the web; I always write all my code assuming that everything will be deleted off the prototype (or replaced with a malicious version) after my code runs :-)

This code predates me, and your PR is changing the attack surface. I agree it's a minor concern in practice, but it's still nonzero.

[papandreou]

Well, okay, I'm not totally invested in this. Just seemed wasteful and O(n^2)-y to create all those intermediate arrays.

[papandreou]

@ljharb, I've rebased this branch now. Still a bit baffled about your initial reaction to this change, do you still feel the same way?

[ljharb]

Made some small tweaks so that now the only runtime dependency is "apply"; I guess this is fine to merge.