Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--compress true - is it a good default? #66

Closed
Andarist opened this issue Jan 25, 2018 · 33 comments
Closed

--compress true - is it a good default? #66

Andarist opened this issue Jan 25, 2018 · 33 comments

Comments

@Andarist
Copy link
Collaborator

IMHO it makes debugging harder, its not easy to look into source code of libraries in node_modules (built by microbundle) nor it is easy to debug microbundle itself because its built by itself with --compress true (default).

I guess the question is what is a primary goal & target of this tool? If producing micro libraries then I think files should be left unminified by default, minifying can be done later by subsequent tools - bundlers etc.

@developit
Copy link
Owner

The goal of microbundle is primarily to package tiny modules that are used in a browser context. In this respect, minification is the most logical default.

@freeman
Copy link
Contributor

freeman commented Jan 25, 2018

Don’t know if it is possible but maybe only the UMD build should be minified, given that the CJS and ESM version mostly imply the consumer is using a bundler ?

@Andarist
Copy link
Collaborator Author

Don’t know if it is possible but maybe only the UMD build should be minified, given that the CJS and ESM version mostly imply the consumer is using a bundler ?

It's certainly possible and what would be a good fit here. I propose 3 possible values of compress option:

  • undefined (treated as auto/default) - compress only umd
  • true - compress all
  • false - compress nothing

@developit
Copy link
Owner

developit commented Jan 26, 2018

Another option that might be worth adding is a way to do --compress pretty, which would run Uglify, but use { output: { pretty: true } }. That would apply all the nice inlining and transformations, but produce readable multiline output.

I'm thinking that might be a sensible default if passing --target node? It'd be performance-optimized, but not compressed.

@Andarist
Copy link
Collaborator Author

Will work on this, once #62 gets merged (it has improved support for tests) - and we will be able to discuss further under the PR about used approach.

@boneskull
Copy link

I don't think it's clear enough this is intended for browser usage due to the default "target" being "node" (this option isn't really explained?)

I kind of assumed it would gracefully handle builtins, but it doesn't.

@vladshcherbin
Copy link

Came here with the same issue when bundling for node.

The goal of microbundle is primarily to package tiny modules that are used in a browser context. In this respect, minification is the most logical default.

Really weird to see this when default target is node. Maybe readme should be updated with this one to avoid confusion and better understanding of package purpose.

@kzc
Copy link

kzc commented Jan 28, 2018

A way to opt out of minification would be useful for debugging.

@lukeed
Copy link
Contributor

lukeed commented Jan 28, 2018

Instead of an all or nothing boolean, can take the same approach as --format and pass a group of formats to compress.

$ microbundle src --format cjs,umd,iife --compress umd,iife
#=> CJS is the only unminified

Not specifying any groups would assume all formats want compression. This way it's not breaking~!

$ microbundle src
#=> all (default) formats compressed

$ microbundle src --compress
#=> (redundant) compress=true ==> all formats

@Andarist
Copy link
Collaborator Author

IMHO this would bloat CLI api a lot. Every other current (and future) option would "need" to support it, adding this to a single option would look weird. It also closes a little bit things like mentioned pretty value (basically any option that could have more than 2 values).

I think microbundle should provide most reasonable defaults with not that many configurable parts (although I don't really believe in 0 configuration tools, so actually I might propose some more advanced ways to customize in the future). If you need such fine-grained control, passing different options' values for different formats etc you can always just have 2 microbundle scripts instead of 1, each targeting specific subset of formats.

@lukeed
Copy link
Contributor

lukeed commented Jan 28, 2018

Hmmm, not so sure that I agree with the idea that current options would need to support this.

Of all our options, only format and compress are the only options that deal with how the output is printed. All others deal with generating the content directly (strict, name, external).

Personally, I'm totally fine with the current compress behavior. My suggestion above is just to accomodate the request in a non-breaking way.

@Andarist
Copy link
Collaborator Author

Imho both strict & external could be considered as those that should support it. Also your proposed option regarding source map generation.

@lukeed
Copy link
Contributor

lukeed commented Jan 28, 2018

Yeah I suppose. Fair enough 😄

@kzc
Copy link

kzc commented Jan 28, 2018

I've never used microbundle, only looked at the docs... I'm assuming that watch mode does not perform minification for quicker dev cycles?

@developit
Copy link
Owner

Watch mode minifies by default. You can always turn off minification by passing --no-compress (in either mode).

@texastoland
Copy link
Contributor

texastoland commented Aug 12, 2018

Watch mode minifies by default. You can always turn off minification by passing --no-compress (in either mode).

It would be helpful to document --no-compress. I tracked it down in the mri tests after checking Sade. The docs say --compress is true by default which means it doesn't do anything without no-.

@lukeed
Copy link
Contributor

lukeed commented Aug 12, 2018

I'm just not a big fan of --no-* flags in general. Intuitively, it feels much nicer to use flags to turn things on rather than to turn them off IMO.

Ideally, here, we'd (1) skip compression for watch mode entirely, (2) enable compression by default for build, and (3) document --no-compress in README or CLI example. And, personally, I still think sourcemaps should be off by default (#73).

@texastoland
Copy link
Contributor

texastoland commented Aug 12, 2018

  1. 💯 yeah.
  2. I agree with your previous discussion about enabling it only for UMD. I don't know if it's common but I directly edit node_modules when I'm debugging a dependency. In Microbundle's case I need to rebuild it because it's minified and uglified.
  3. I think all that's really missing are example()'s in general. I'm having trouble with -i too.

@ForsakenHarmony
Copy link
Collaborator

yes, I feel like a lot of people are using microbundle to publish to npm and I don't really see minification as a good default there, had problems with it myself, especially before I fixed sourcemaps

@lukeed
Copy link
Contributor

lukeed commented Aug 12, 2018

re: 2) I actually feel that compression should be off for build by default as well. I wrote the above in the mindset that the majority of users may expect build to be minified by default. Not sure if that's the case.

@developit
Copy link
Owner

I ran a Twitter poll, and it looks like web is the majority of microbundle's use-case. That means the majority of Microbundle users expect minification by default - I'd actually suggest that's one of the primary reasons people use this tool.

https://twitter.com/_developit/status/1029460914897076224

Some things to consider:

Defaults are everything

Microbundle has a fair number of users already, and the reason they're using it is because of the default output. Changing the default output changes the project. There are lots of other alternatives people can/could choose, so if folks are using Microbundle it's very likely because of the simple optimized-by-default output.

I think --target node is really our chance to add an additional use-case for Microbundle, because it's the only way to avoid ruining the existing defaults for web.

Not all minification is equal

It is exceptionally rare to see developers enable property mangling when minifying their applications. Microbundle incorporates configurable property mangling that is rather difficult to set up using Uglify directly, as evidenced by the fact that most library authors don't use it. I would be inclined to make property mangling easier before removing minification by default.

Microbundle is actually a size tracking utility too

Size is an important metric for library authors, and showing size without minification and compression is meaningless. Applications that care about performance minify and compress their JavaScript, and the size relationship there is non-linear. Anyone authoring a library that is used in a browser environment has a responsibility to track the minified + compressed size of that library so that their users can make informed decisions about the cost efficacy of installing their module. Since Microbundle caters specifically to that use-case, the responsibility falls to us.

Watch mode

About --watch mode - I've never viewed Microbundle's watch mode as being analogous to something like Webpack's. I use --watch to track compressed size of codebases I'm working on as I make changes, so that I don't accidentally regress size. Since we don't provide a server component do directly host watch mode files in the browser, I'm not sure the compilation speed of watch mode is important enough to justify making it useless for size metrics?

re: documetation - I definitely agree that the help output and README should document flags using their "on" values ("--no-compress : turns off minification"). We can also create a table that clearly shows the default values for --target node and --target web (default).

@ForsakenHarmony
Copy link
Collaborator

Are you sure people expect minification when they target browsers?

I definitely target browsers with parket, but I never really wanted it to be minified

@Andarist
Copy link
Collaborator Author

I agree with your previous discussion about enabling it only for UMD. I don't know if it's common but I directly edit node_modules when I'm debugging a dependency. In Microbundle's case I need to rebuild it because it's minified and uglified.

You are not alone, I edit node_modules a lot too 😉

yes, I feel like a lot of people are using microbundle to publish to npm and I don't really see minification as a good default there, had problems with it myself, especially before I fixed sourcemaps

Are you sure people expect minification when they target browsers?
I definitely target browsers with parket, but I never really wanted it to be minified

👍 Same here, main target is browser but at the same moment the main target are modern bundlers and I don't want to feed them with minified source (mainly because harder debugging & node_modules editing)

Defaults are everything

I believe that is the main reason why people use microbundle - because it's 0 config and most people struggle with configs because usually they need deeper understanding of the tool. This doesnt immediately imply that current defaults are what people actually want.

Not all minification is equal

Agreed, but maybe this should be responsibility of another tools? uglify presets or smth?

Microbundle is actually a size tracking utility too

Tracking size doesnt have to happen on directly distributed files though. I.e https://github.com/TrySound/rollup-plugin-size-snapshot allows for tracking minified+gzipped code but it doesnt change the output files in any way (so they stay unminified).

@freeman
Copy link
Contributor

freeman commented Aug 17, 2018

I answered the poll but like andarist I target browsers through bundlers so I don’t expect minification except for the umd build (for unpkg usage).

@nathanforce
Copy link

Just to chime in, I chose Microbundle because of the zero config.

I use it to develop React libraries that are ultimately targeting the browser via a bundler. Would love a best of both worlds where minimal/zero config is maintained but industry standards are achievable for minification etc.

Loving the simplicity of using Microbundle!

@lukeed
Copy link
Contributor

lukeed commented Aug 20, 2018

Our conversation is kinda split, but I'll come back here since this is where most of the chat lives :)

Good points all around, and I'd totally be a happy camper any way forward. I think the best combination of behaviors, spanning all points, is perhaps the following:

  1. watch mode is always uncompressed, by default

    @developit can regain his size-over-time tool simply with microbundle watch --compress. I feel a lot of the concern around compressed development is around the inability to see what the built version of one's library actually looks like -- for example:
    a) did tree-shaking work correctly
    b) did that dependency map correctly
    c) are my exports how I want them, etc.

    I tend to have these questions a lot

    I think there are also some in the camp of not wanting to wait for Uglify to run every time, but that's probably a much smaller group.

  2. build is compressed, by default

    Since most of the users area developing browser libraries (as expected), this doesn't break their deliverables. It also stays true to the zero-config & good defaults that people come for.

  3. Better use of --targets

    I feel that that --target flag is kinda neglected & not really talked about, but it's the answer to this debate.

    Microbundle should continue to default to --target web (see (2) above), which builds with minification and source mapping by default.

    By contrast, --target node will not run build with compression or source mapping.

    Each target follows the "standard" for that context & the compress/sourcemap flags can easily override the current target's default setting.

@texastoland
Copy link
Contributor

texastoland commented Aug 20, 2018

My opinion is --compress isn't a great default for any target. Even for browser npm ought to publish uncompressed for bundling and debugging. An additional .min.js or something makes sense.

@Andarist
Copy link
Collaborator Author

Even for browser npm ought to publish uncompressed for bundling and debugging. An additional .min.js or something makes sense.

+1 for both

Microbundle should continue to default to --target web (see (2) above), which builds with minification and source mapping by default.

I'd like to introduce a special process.env.BROWSER in the future that would be replaced automatically for such builds with rollup-plugin-replace, so in my opinion this --target web (although I would prefer browser to better match "browser" field in pkg.json) shouldnt enable compression by default (for reasons given by @texastoland above and this one)

@Andarist
Copy link
Collaborator Author

Andarist commented Sep 11, 2018

Idea

What if w could satisfy both worlds and just provide both minified & unminified versions by creating a proxy entry such as this

import * as dev from "./dist/${packageName}.development.mjs"
import * as prod from "./dist/${packageName}.production.min.mjs"

${exports.map(name => `

export var ${name} = process.env.NODE_ENV !== 'production' ? dev.${name} : prod.${name}

`).join('\n')}

(snippet originally authored by @TrySound, this would have to handle default export differently, but it's easy to adjust this)

Downsides

If the package is not entirely tree-shakeable then the consumers would have to probably pay twice the price of that (unused things in both prod & dev bundles would have to be added to the final bundle). OTOH w can detect such a situation & warn/educate people on what they can do to make libraries tree-shakeable.

@mikestead
Copy link

Reviving this one as was a tripped up. As perhaps hinted at earlier, could we let the filename of each target define its requirement? So if suffixed with .min.<ext> then the prod build will minify it?

@ianstormtaylor
Copy link

Hey @developit, running into this as well. I think @lukeed has a point about allowing for different compression configurations by format. But I also recognize your goals which are super valid in terms of ease of use and bundle size.

  • It should easy to use out of the box.
  • It should make it easy to arrive at micro bundle sizes.
  • It should bundle libraries as expected for the browser and node. (And isomorphic!)

I think there's a way to satisfy all of those...

Change the compression defaults.

One of the issues is that people expect to receive non-minified code in the main and module files from package.json. This is because these fields are expected to be used with a bundling tool, and you want to preserve the source non-minified to make development easier.

(Yes sourcemaps help a bit, but they don't help when you need to edit files in node_modules when needed, which is a common use case. There are also probably other valid use cases for non-minified packages.)

But at the same time, people also assumed that there is a minified UMD build available to quickly use on the web.

  • umd should be compressed by default.
  • es, cjs and modern should not be compressed by default.

Change the bundle size output.

The other issue I think is that the bundle size print outs from the CLI once for each type of format used. This makes sense from a mental model standpoint, and is easy to implement, but it's also not exactly the best solution for many of the use cases.

As a user I have to read through three extremely similar sizes—similar enough that I don't really care about the minute differences. And none of the sizes are the "true" cost of the module anyways, because that size depends on what it is bundled with—they're just an estimate.

I propose that we instead print out only one estimated size. And that that size ignores the wrapper parts of the bundling code, giving you the "truest" size estimate for the package's impact on a bundle. For example:

Built "library": 2.62 kB (min+gzip estimate)
     - library.js
     - library.es.js
     - library.m.js

I think this would actually be a UX improvement when using --watch to monitor sizes, since you wouldn't have to remember which you were mentally tracking. And it would make it easier to copy-paste the bundle size into Readme's and other places for marketing.

(You could always have a --verbose option that retains the current logic of printing out all of the sizes. But it's really not actually useful for the 99% use case.)


Those two changes allow compression to be toggled by format. And by decoupling the size calculation from the individual files output you can maintain consistent size tracking regardless of flags being toggled.

At least that seems to solve everyone's concerns I think.

@devcat22
Copy link

This makes any library that uses microbundle insanely difficult to debug. Please add this option.

@developit
Copy link
Owner

Apologies for letting this issue drop - there are a few open issues/PR's discussing the topic, I think this one just got buried.

@ianstormtaylor I totally agree on the size display thing. I think it would be reasonable to have Microbundle only output the size for the ESM build, unless that build is specifically omitted via --formats. The other 2 sizes are directly related to this, so it does mainly add noise. The modern size is a bit of a different case here, but that might be a reasonable area where we just allow passing --size modern or something, so folks can choose what to track.

Regarding process.env and .production.js VS .development.js: I'm really hesitant about both of these workarounds. Neither of them function properly in all bundlers, and neither of them work with native ESM.

I've left some comments over on #187 with potential ways this can be moved forward, for those interested. The TLDR is that package authors can be reasonably confident that their users will have basic whitespace and identifier mangling in place, so we should do property mangling and optimization/inlining in Microbundle but can preserve whitespace and identifiers in dist files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests