Skip to content
This repository was archived by the owner on Dec 31, 2020. It is now read-only.

Do not minify bundles#759

Merged
mweststrate merged 1 commit intomobxjs:masterfrom
vkrol:do-not-minify-bundles
Jan 12, 2020
Merged

Do not minify bundles#759
mweststrate merged 1 commit intomobxjs:masterfrom
vkrol:do-not-minify-bundles

Conversation

@vkrol
Copy link
Copy Markdown
Contributor

@vkrol vkrol commented Aug 11, 2019

I think it's a good idea not to minify bundles. I think it's very convenient to be able to edit the module in node_modules directory and it's much easier to do when the script isn't minified. Also source maps can be disabled in dev mode for some reason and not minified bundle can help in this case.
I also disabled source maps generation because I don't see the point in it if we don't minify the bundles.

Also:
@FredyC

I don't think you should be providing a minified code as a main, that should be a job for the consumer
#644 (comment)

@danielkcz
Copy link
Copy Markdown
Contributor

danielkcz commented Aug 11, 2019

Well, that's a fairly old comment of mine and I had a change of heart since then.

The UMD has to be minified because consumer cannot do that.

I think it's very convenient to be able to edit the module in node_modules directory and it's much easier to do when the script isn't minified.

That's a very weak reason imo. How often do you need to do that?

Personally, I don't like microbundle output much. I prefer TSDX which has output like this...

image

https://unpkg.com/browse/formik@1.5.8/dist/

There is index.js which picks either dev or prod bundle based on NODE_ENV. The UMD exists in both variants too. The ESM is single one unminified because that one is usually taken care of by bundlers.

@vkrol
Copy link
Copy Markdown
Contributor Author

vkrol commented Aug 11, 2019

I had a change of heart since then.

Would you mind explaining why?

The UMD has to be minified because consumer cannot do that.

I think we can minify the UMD bundle only in that case.

That's a very weak reason imo.

I think that's a very strong reason. This ability encourages experimentation and this is one of the most important advantages of the modern JS ecosystem. If the bundle hadn't been minified, this PR #711 would have been made a couple of weeks earlier ;)

How often do you need to do that?

I needed this two months ago when I tried to upgrade to mobx-react@6 at my job.

Also: #128.

@danielkcz
Copy link
Copy Markdown
Contributor

Would you mind explaining why?

Simply because I did not know that much about bundlers and ecosystem back then.

Anyway, that's why migrating to TSDX could be a better option as it provides all variants to satisfy everyone.

@vkrol
Copy link
Copy Markdown
Contributor Author

vkrol commented Aug 11, 2019

Simply because I did not know that much about bundlers and ecosystem back then.

OK, thanks.

Anyway, that's why migrating to TSDX could be a better option as it provides all variants to satisfy everyone.

I'm not sure about switching to TSDX right now. I'd like to set up the microbundle appropriately as a first step and then explore the possibility of switching to TSDX. What do you think about it?

@danielkcz
Copy link
Copy Markdown
Contributor

danielkcz commented Aug 11, 2019

I am not sure what do you want to set up with microbundle :) It works currently. I don't think it's a good idea to just drop non-minified bundle out of whim. And removing source maps is totally out of question. It's still compiled file, source maps need to be there.

Btw, for any kind of serious experimentation, I recommend https://github.com/whitecolor/yalc instead of editing node_modules directly. I've learned that lesson painfully enough when I changed something, then needed some additional dependency and that deleted my changes.

@vkrol
Copy link
Copy Markdown
Contributor Author

vkrol commented Aug 11, 2019

I am not sure what do you want to set up with microbundle :) It works currently. I don't think it's a good idea to just drop non-minified bundle out of whim.

I want the following:

  1. CJS and ESM bundles are not minified
  2. UMD is minified

As a confirmation of the viability of my idea, you can see the number of comments on this issue developit/microbundle#66.

And removing source maps is totally out of question. It's still compiled file, source maps need to be there.

OK.

Btw, for any kind of serious experimentation, I recommend https://github.com/whitecolor/yalc instead of editing node_modules directly. I've learned that lesson painfully enough when I changed something, then needed some additional dependency and that deleted my changes.

Thank you. For the serious I agree, but for simple cases, it's overkill.

@vkrol
Copy link
Copy Markdown
Contributor Author

vkrol commented Aug 11, 2019

I want the following:

  1. CJS and ESM bundles are not minified
  2. UMD is minified

Done.

@vkrol
Copy link
Copy Markdown
Contributor Author

vkrol commented Aug 11, 2019

vkrol force-pushed the vkrol:do-not-minify-bundles branch from 71c013b to b9d7991 now

Got rid of some duplication in the bundle-cjs-and-es and bundle-umd scripts.

@danielkcz
Copy link
Copy Markdown
Contributor

danielkcz commented Aug 11, 2019

Let's leave this open for a while so others have a chance to say something about it. It's Sunday evening after all :)

Btw, you need to tweak bundle size config...

@vkrol
Copy link
Copy Markdown
Contributor Author

vkrol commented Aug 11, 2019

Let's leave this open for a while so others have a chance to say something about it. It's Sunday evening after all :)

OK, thanks. Of course :)

Btw, you need to tweak bundle size config...

Thanks. I will do it.

@vkrol
Copy link
Copy Markdown
Contributor Author

vkrol commented Aug 11, 2019

Btw, you need to tweak bundle size config...

Done https://github.com/mobxjs/mobx-react/pull/759/files#diff-445c4a6aec537e749519a382896d04beR3.

@vkrol
Copy link
Copy Markdown
Contributor Author

vkrol commented Aug 21, 2019

I don't want to bother you, but I'd like to know if we should wait any longer. It's been nine days.

@danielkcz
Copy link
Copy Markdown
Contributor

Yea, I don't like merging these "breaking" changes without anyone else having a look. If it would be up to me only, I would go with TSDX which I know and offers the best from both of the worlds.

@vkrol
Copy link
Copy Markdown
Contributor Author

vkrol commented Aug 21, 2019

Okay, we'll wait. Thanks.

@vkrol
Copy link
Copy Markdown
Contributor Author

vkrol commented Aug 29, 2019

@mweststrate @xaviergonz @Sunshine168
I'm sorry, I don't want to bother you, but I'd be very interested to know what you think about these changes.

@mweststrate
Copy link
Copy Markdown
Member

Honestly, with these kind of things, one has to have very good reason to deviate from the community best practice before applying deviation from standards like this.

Ideally I'd just follow whatever microbundle recommends, and have the discussion there rather than on a single package, championed by just a single person for a quite unorthodox use case :) (I don't mean that in any offensive way, I just know from experience that when maintaining a package, changing things for a one-off request often just result someone requesting the exactly opposite two months later). And especially when it comes to build setup, changing something, blows always something up for someone. (In this case, for example, someone might not be minifying his own code, or minifying with settings that don't align with our code base, etc, etc).

So I'd rather have the voice of more consumers before making a change like this.

That being said, I actually don't know from top of my head what we do in other repositories in this namespace, it could very well be that mobx doesn't minify either. In that case there would also be the argument of consistency.

@danielkcz
Copy link
Copy Markdown
Contributor

danielkcz commented Aug 30, 2019

The mobx and mobx-state-tree do both 😆 The mobx-react-lite is a copy of mobx-react so it does minify (but there is mobxjs/mobx-react-lite#212 to change that).

https://unpkg.com/browse/mobx@5.13.0/lib/
https://unpkg.com/browse/mobx-state-tree@3.14.1/dist/

However, looking at package.json the minified version is not referenced. So in a sense not minifying is current "standard" for mobx.

Copy link
Copy Markdown
Member

@mweststrate mweststrate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is fine, beyond UMD i'm pretty sure there is in practice no need to minimize the other builds, as webpack etc handle that themselves. The only reason to do minification upfront is to be able to use extra risky strict minification settings (e.g. mangling property names, which is in generla not safe, but for some libraries might be).. Be we don't do that in our case anyway, so I think this approach is fine :)

@mweststrate mweststrate merged commit b4658a0 into mobxjs:master Jan 12, 2020
@vkrol
Copy link
Copy Markdown
Contributor Author

vkrol commented Jan 12, 2020

Hooray! Thank you!

@vkrol vkrol deleted the do-not-minify-bundles branch January 12, 2020 21:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants