developit / microbundle

📦 Zero-configuration bundler for tiny modules.
https://npm.im/microbundle
MIT License
8.03k stars 362 forks source link

Compression is enabled by default #937

Closed mindplay-dk closed 2 years ago

mindplay-dk commented 2 years ago

I made a guess, and it turns out there's a --no-compress option to disable compression.

I don't know why the --compress option exists? Compression is enabled by default, so it doesn't have any effect.

changeset-bot[bot] commented 2 years ago

⚠️ No Changeset found

Latest commit: 11e4fb1758464c53c1f048ebb538ac0e70482953

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

rschristian commented 2 years ago

Whether compression is enabled by default or not depends on the --target. By default Microbundle targets web, which does enable compression. Switch that to --target node and compression is disabled).

Furthermore, we really shouldn't be documenting the inverse of a flag.

--no-<flag> is just a common unix pattern for disabling boolean flags, it's not a separate option. --compression false, --compression=false, and --no-compression are all equal. It's just whichever you prefer really.

mindplay-dk commented 2 years ago

--no-<flag> is just a common unix pattern for disabling boolean flags, it's not a separate option. --compression false, --compression=false, and --no-compression are all equal. It's just whichever you prefer really.

I had no idea. I'm not a Linux person.

This should probably be mentioned in the README, then.

rschristian commented 2 years ago

Was/is it not clear to you that is a boolean flag? I agree, we should specify defaults as that is missing from --compress.

mindplay-dk commented 2 years ago

Was/is it not clear to you that is a boolean flag?

No, that was clear - I just had no idea that the no- prefix worked in general. I'm suggesting the README should probably mention that these options exist. This might be something Linux users can guess, but I'm a Windows user - I don't do much of anything on the command line if I can avoid it.

rschristian commented 2 years ago

This isn't a Linux pattern, but a Unix one, and certainly plenty of Windows apps having taken inspiration.

This isn't something we've set up but what sade does automatically. I don't think we need to write out all the ways you can negate a flag.

If it was clear it was a boolean you should be fine. --compress false is the same thing.

rschristian commented 2 years ago

@mindplay-dk I just opened #940, if you have a moment to take a peek I'd love to hear if that would help.

Also remembered while writing it that sade supports --compress 0, so there's a lot of possible ways to represent boolean flags. Covering them all therefore seems just a bit silly to me, when the normal (and likely first thought) --compress false should work just fine for everyone.

mindplay-dk commented 2 years ago

@mindplay-dk I just opened #940, if you have a moment to take a peek I'd love to hear if that would help.

This clarifies the defaults, which is helpful for sure, but that was not my issue. 🙂

I know you think this is just something everyone knows - but there's technically no standard, as far as I'm aware, these are just de-facto conventions. There's literally dozens of CLI libraries and option parsers for node - they don't by any means have the same capabilities or follow all the same conventions; and these libraries are generally an implementation detail, not a documented fact.

Would it hurt to have a line somewhere like:

To disable an option, use for example `--no-compress` or `--compress false`, etc.

That way, you've committed to supporting that feature - like, if you were to switch to a different CLI library, or your CLI library switches to a different option parser for some reason.

Also remembered while writing it that sade supports --compress 0, so there's a lot of possible ways to represent boolean flags. Covering them all therefore seems just a bit silly to me, when the normal (and likely first thought) --compress false should work just fine for everyone.

Honestly, the --no prefix was a complete shot in the dark for me. I just figured there must be some way to turn this off and they forgot to document it.

Laugh if you must, but I have 23 years of professional developer experience, and I just learned about this. I've never much liked the command-line. I know it's powerful and flexible, but it's also error prone, and I don't have the type of mind that does well at rote memorization.

Moreover, is it reasonable to assume that juniors who are just starting out with JS tooling would be able to guess that there are command-line switches which aren't documented?

Just my two cents, but if we want to be inclusive when writing documentation, we need to be mindful of people who don't have any of the implicit knowledge we've picked up over the years, don't have the same mindset, the same skills, and so on.

Sorry if I seem hell bent on this, but I just think inclusivity is super important if you want a community to grow and evolve - and if you want a tool to succeed. 😄

I'll shut up now.

rschristian commented 2 years ago

I know you think this is just something everyone knows

No, not at all.

What I think is that this isn't something that everyone needs to know, as "we" (read: mri and sade) have plenty of other ways of supporting this behavior including the much more obvious --<something> false. It's not at all important to the usage of this tool that every user knows every supported format for setting boolean flags. 1 is perfectly workable.

I'd take this to sade instead of here. This isn't something we handle, it's not something we've set up. Furthermore, if you think it's a real issue for inclusivity, this should probably be distributed to all users of sade, no?

That way, you've committed to supporting that feature

I don't think there's anything at all related to the CLI that we'd want to commit supporting, much less 1 of the 4 different ways to set boolean flags. If a change needs to happen for whatever reason, we can increment the appropriate SEMVER version number and continue on.

Moreover, is it reasonable to assume that juniors who are just starting out with JS tooling would be able to guess that there are command-line switches which aren't documented?

It's not a separate switch, just a different way to set the value.

Just to be clear, as I understand it can be hard to convey tone through comments like this, I'm not looking to make this an esoteric tool in my (very limited) capacity to do so; I just don't think it's particularly useful and/or relevant to document all the possible combinations in setting a value, as A) that itself can be overwhelming to new users and B) it's not something even something built/supported here. If you have an issue with the "magic" that the the CLI does, it seems to me that bringing the criticism to a consumer of that lib is not particularly productive.

This whole thing just seems really odd to me as if we removed --no- from all examples, you'd have no issue whatsoever. Even less documentation would be better in this scenario it seems.

mindplay-dk commented 2 years ago

I would have never thought to use --something false as the CLI in generally just text - it doesn't have types, and there's no logical reason to choose a keyword like false over no.

I don't think it's realistic to expect users to examine the innards of your CLI implementation before they try to use it - to figure out that you're using sade (which I had never heard of) and reference it for different ways to toggle flags.

For the record, this wasn't about winning an argument. I'm just trying to make an argument for making this more accessible to other users. But no point in carrying on with this. I've presented my arguments. You do you. 🙂