qs
qs copied to clipboard
ljharb
Size increased greatly in 6.10.0 due to getSideChannel
I happened to check lib size in Bundlephobia before updating package and noticed the size has increased from 10 kB minified to 30 kB minified. From lib user viewpoint this is not an acceptable change.
Commit 7c1fcc53 dist/qs.js exposes it was the addition of getSideChannel that caused this.
That's the minimum code required for correctness.
Bundlephobia is highly misleading - the only size delta that matters is in your app's resulting bundle, and that can't be estimated reliably without your entire app.
If, for example, your app only supports environments that support WeakMap, you could configure your bundler to replace the entire side-channel package with:
const inspect = require('object-inspect'):
function assert(key) {
if (!this.has(key)) {
throw new TypeError('Side channel does not contain ' + inspect(key));
}
}
module.exports = () => {
const channel = new WeakMap();
channel.assert = assert;
};
You can even eliminate the object-inspect require if you don't care how useful the error message is.
That seems like the opposite way of doing it, opt-out instead of opt-in. Usually when using possibly unsupported features you let people know that hey, we depend on this modern feature, provide a polyfill/ponyfill for it. So far this library has clearly had a zero dependency policy and looking at the API the traditional way to provide an alternate WeakMap (or side channel) would've been to let user provide it via options.
It's the safest. By default, it works in the maximum number of environments. For those who care about bundle or code size and know how to maintain correctness - the minimum by a large margin - they can opt in to all sorts of optimizations to achieve that.
This library may have had a "zero-dependency" policy prior to my taking it over, but I would never have such a foolish and short-sighted policy on any software I maintain - more deps is better.
I would certainly accept a PR that allows tests to pass (and addresses #403, which I'm looking into now) without needing this dependency, but I'm relatively confident that wouldn't be possible. "Needing a polyfill" is a breaking change, and I'm not going to do a semver-major bump because a negligible amount of kilobytes gets added to a bundle that's gzipped over the wire anyways.
I do agree as well that an alternative to "handling cycles by default" would have been "add an option that injects some kind of side channel for handling cycles", but that would be much less user-friendly.
However at the moment you provide a lightly customized variant of WeakMap to everyone while only a tiny minority actually needs it. But I admit I'm mostly looking this from the perspective of the front end: in there being picky and preferring less dependencies is often better, while the typical issue is how people add extra deps even when they're not really needed. And the current trend of doing everything via JS doesn't really help. Quite a bit different world and I have a strong bias so it is hard for me to judge how foolish zero dependencies is, or if "more deps is better" would be foolish.
In my project's particular case we will eventually remove qs and use URLSearchParams instead since we don't really need the special features provided by this utility, and the dependency was likely added as it was a familiar tool to an earlier dev in the project.
Regardless it is great that you spend time on maintaining open source.
That's a perfectly reasonable choice as well - although USP doesn't support nested params, which is a very commonly needed feature in web dev.
The replacement I provided above seems like it should be more than sufficient if you only support WeakMaps.
I would definitely consider an option to disable cycle checking, though, and do a conditional require so that your bundler wouldn't even include side-channel unless you wanted it.
would you be willing to ship multiple builds? we could do a build without the polyfill and one with. This is a PR I would be happy to spend some time on.
Not to mention, I'd also be happy to help update the build process to support outputing commjs and es module formats as well as UMD
UMD is obsolete, and imo it’s harmful to output it since it enables legacy build pipelines. As for ESM, that doesn’t provide any benefit at all unless there’s named exports to be had, and this package doesn’t have any.
I’m not sure how multiple builds would help, since there’s no convention or automated way for end users to select the build they want. If end users have that willingness and level of build control, then they can already alias side-channel to something smaller.
I’d be very willing to provide an ESM entrypoint in side-channel that’s much smaller, since ESM syntax can indeed assume the presence of WeakMap, but I’m not sure how helpful that would be.
There's issue with esbuild not being able to include qs because of side-channel depending on call-bind, which breaks during bundle.
@Inviz then please file an issue on esbuild, because call-bind is used on a significant percentage of npm’s downloaded packages.
Good job on maintaining the lib, and as a fellow open source affectionado I appluad you for doing a much needed work. QS library that supports nesting is really a must-have.
Though I'd like to say, that reading the tone of the comments in this thread leaves a bit of an aftertaste. I understand that we all do things we do for passion, and we don't want anybody telling us what to do in our free time for free, but at times the magic of open source is meeting each other in the middle, while keeping the communication friendly.
I had to revert to earlier qs (thanks to details outlined in this issue), not a statement, but more of a necessity. My project was not building on esbuild (which i am not even sure the issue with call-bind itself, i think it's more about how it is bundled into qs), and frontend bundle was larger than necessary. Instead of coercing maintainers of multiple projects to remove the dependency and figure out what's wrong with the build, it was easier and more productive to opt-out of the problem entirely.
Either way, best regards.
@Inviz I totally understand that. esbuild, however, is a very new project, and I'd think it's expected that it has a number of roadblocks before it catches up to what most other bundlers have had the better part of a decade to account for.
If you can file such an issue for call-bind on esbuild, and link it here, I'm more than happy to work with the esbuild maintainers (as i've done before) to help figure out the best course of action to fix esbuild.
The problem with this size increase is that as I can see the trend has been to downgrade the lib to solve the issue, out of all the packages I use, 4 use qs and all rolled back to qs 6.9
So I can imagine none of those packages will ever upgrade the qs version (even if bug fix or security fix) unless this issue is solved, because putting the burden of optimizing a lib of a lib to an end user is the kind of thing that never happens and for good reasons, how would they know if not explicitely heavily documented in the lib? And yet they're the only ones to have this power in the final build (unless the lib is bundling qs)
That's why less dependencies is usally better, and sure adding a dependency to not reinvent the wheel is great but it shouldn't be a reflex for the simplest of feature especially if they have a native alternative, because most users don't want to have to deal with deeply nested dependencies problems and optimizations. General rule: ship libs with no polyfills and let end users polyfill them, that's why side-channel is a bad dep because it's a polyfill in it's entirety
So yes the question is what's the best course of action now? Can we just use WeakMap directly instead of side-channel using the example code you provided, and let babel-polyfill it if it's needed by the target env like always?
@Tofandel which four?
side-channel is not a polyfill in any way; its an abstraction layer. It doesn't provide WeakMap or Map when they're absent, it just uses them when they're present.
WeakMap is impossible to polyfill, and requiring Map to be polyfilled would be a breaking change - and those are always much more costly than a size increase.
The libs would be Express, Ziggy, Inertia.js and Algolia
It's impossible to polyfill perfectly but not impossible to polyfill partially, it's all down to the use case qs makes of it, in this case https://github.com/polygonplanet/weakmap-polyfill would work from the usage I gathered, the babel polyfill also works but is much bigger
And well, let's just say given the very good support for Weakmap (96%+) it doesn't make much sense to polyfill it by default, though that might bump requirement of node to >v7 (but maybe it's time given v12 is almost EOL already)
I had a look at where the size increase comes from and its the use of get-intrisic, by itself it's 10Kb and not optimizeable, and it's basically only there to check for if absent or not.. I'd recommend a different method
What's really ticking for me, is the unperfect polyfill for Weakmap is less than 2Kb, but the lib to check if absent is 5 times that
Maybe then we should just have a dependency on the polyfill and really simplify barebone what's used in qs with this polyfill and using the original Weakmap if it exists, like this the bug stays fixed with no breaking change but the size increase is only of ~3-4Kb
Express is on v6.9 only because of the ^ dependency range used; in no way is the size a barrier - I've spoken to the primary express maintainer about this before. I can't speak to the other three.
Express is on v6.9 only because of the ^ dependency range used; in no way is the size a barrier - I've spoken to the primary express maintainer about this before. I can't speak to the other three.
And even then, it was never rolled back, just not yet upgraded, though it will be soon, as we don't want to get stuck behind, of course.
Just thought I'd clarify that point; I don't have any opinion on the actual points of the issue itself.
Yes sorry, for express size would pretty much not be an issue (but its pinned to 6.9.7 though), it was in my npm explain so included in the 4 librairies I use, the other 3 I know for a fact they kept it to 6.9.7 because of size increase
Anyways this is getting outside the point, I'm just saying if we can save 10Kb by doing a typeof Weakmap === undefined ? require('weakmap-polyfill) : Weakmap; instead of using side-channel without breaking anything then it should be a no brainer
@Tofandel if that's actually achievable, then i'd be happy to review such a PR to side-channel itself - it's used in a ton of packages, not just this one. However, the problem is that it needs to work as performantly as possible in all of:
- implementations with native WeakMap
- implementations without native WeakMap but with native Map
- implementations without a native WeakMap or Map
As promised, PR is done, it's breaking change free and reduces the size of side channel by 19Kb, that is if using the require('side-channel/withoutInspector.js) file with the inspector it would be reduced by only 10Kb
For the poor souls who just want to stringify some basic nested objects, this library is vastly over-engineered - I honestly don't know who needs that.
If you are careful with the input, qs-stringify does the job in <200 bytes (only basic object/values, not cycles-safe).
@elsassph most people need it, because most web server frameworks require a format that this library provides (and URL does not).
It's true, i need to adjust arrayFormat and other things like that, used for structural parsing (not just K/V)
On Tue, Jan 23, 2024 at 1:27 AM Jordan Harband @.***> wrote:
@elsassph https://github.com/elsassph most people need it, because most web server frameworks require a format that this library provides (and URL does not).
— Reply to this email directly, view it on GitHub https://github.com/ljharb/qs/issues/404#issuecomment-1904473657, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAVFBV567E5OZDTTN2JJ3YP2OQJAVCNFSM4ZWULTB2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOJQGQ2DOMZWGU3Q . You are receiving this because you were mentioned.Message ID: @.***>