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

Merge with Size Limit #26

Closed
wants to merge 8 commits into from
Closed

Merge with Size Limit #26

wants to merge 8 commits into from

Conversation

ai
Copy link

@ai ai commented Jul 3, 2017

Let’s merge best parts from Size Limit and bundlesize!

Since bundlesize is better name, I move all Size Limit code to this project.

I tried to copy code style, but if I lost something, let’s fix it after merge :D.

@siddharthkp please, do not delay with answer. It requires a lot of energy to decide to merge projects. I had dramatically weekend, because I created Size Limit before bundlesize. But my mistake was to not promote it in the next day.

@Furizaa sorry, my PR could have conflicts with yours. I saw that yours is not ready yet, so I just copy tests from Size Limit.

@ai
Copy link
Author

ai commented Jul 3, 2017

Why I add webpack?

  1. A lot of projects has no one build file. For example, PostCSS. We publish a lot of small files.
  2. Many projects has own npm dependencies. Of course, they load this dependencies by require(). And we should count them as part of bundle too. In my opinion, dependencies with unexpected size are the main problem of libraries size.
  3. Webpack inserts a lot of unexpected polyfills. And because library users will add libraries by webpack to own bundle, we should test it. For example, one wrong process usage could increase bundle size in few KB.

A lot of projects use Rollup. Do they need webpack?

With webpack we test not size of library bundle. We test how this library will increase app bundle, when user will add library to application.

I think this is better test, since it is main question, that users ask. And since checking just bundle size will not find many problems (like huge library dependencies or wrong process usage).

So it is OK to put Rollup bundle to webpack, since all library users will do the same.

Why you copy code and not improve existed bundlesize code

I really thought to just improve bundlesize code. But I found few very big problems:

  1. No Node.js 4 support. A lot of projects runs Node.js 4 on Travis CI (PostCSS, for example). Node.js 4 still supported. So it is important, in my opinion, to support it too.
  2. bundlesize uses sync method, which are bad for performance.

What features do this PR add to bundlesize

  1. Webpack bundles files and UglifyJS minimize them. It is important because many projects (PostCSS for example) has no bundle file in repo.
  2. bundlesize FILE to see bundle size. It is important when you have no bundle file in repo. So you need to get size before write limit to package.json.
  3. bundlesize will run only in first job on CI (if project is tested in difference Node.js versions, bundlesize will run only on first Node.js version). Gzip is expensive call. So we need to be respectful for common resources such as feee CI services. This is why it will be respectful to run bundlesize only on first CI job.

Does this PR could be released?

I try to keep old API. But we need to do 2 things before release:

  1. I test only Size Limit code (I copy tests from Size Limit). So we need to test all GitHub Build code.
  2. I have no npm@5 right now. So I can’t generate package-lock.json. Can somebody help me with it?
  3. We need to fix prettycli to support Node.js 4 Fix for Node.js 4 prettycli#1

@Furizaa
Copy link
Contributor

Furizaa commented Jul 3, 2017

The PR #20 is basically ready. It should work without breaking changes. The only thing missing are a few tests that can be added at a later date. I just waited to get a general feeling from @siddharthkp if I should continue with this path - or if the approach goes into the wrong direction before I could invest more time.

That said - I will not be able to update my PR after this has been merged and it had to be closed. Our approaches conflict deeply. But I also would be OK with that as it was a shot into the dark anyway. Just a little saddened because of the lost Sunday ;)

@ai
Copy link
Author

ai commented Jul 3, 2017

@Furizaa I understand your feelings about Sunday. I lost whole previous weekend because late promotion ;). But you did really great job.

I can try to backport your tests here if @siddharthkp will agree with merge plan. Anyway we need your great tests to check all GitHub build features.

@siddharthkp
Copy link
Owner

@ai Sorry, but I don't think it makes sense to merge the 2 projects.

As you pointed out, they solve 2 different problems.

bundlesize solves for library sizes and hence is not opinionated on how you build and minify your library. This is intentional.

size-limit on the other hand focuses on application bundles, hence using webpack to bundle makes more sense.

I commend your effort here, but I do not want to change the premise of the tool itself.

@siddharthkp
Copy link
Owner

I had dramatically weekend, because I created Size Limit before bundlesize. But my mistake was to not promote it in the next day.

I'm really sorry, I did not intend to steal your idea. If there's something I can do to help (while keeping the core idea of bundlesize the same), please let me know!

@ai
Copy link
Author

ai commented Jul 3, 2017

bundlesize solves for library sizes and hence is not opinionated on how you build and minify your library. This is intentional.
size-limit on the other hand focuses on application bundles, hence using webpack to bundle makes more sense.

Nope :D. Size Limit is focused on libraries, too. It is not focused on application bundles.

Size Limit use webpack, because webpack users will use webpack to add library to their projects.

So if dist/mylib.min.js is 100 KB, when user will add mylib to his application, it application size will not increase by 100 KB. mylib could use process in wrong way, so application will be increase by 110 KB because of this library.

Or mylib could have dependencies for 1 MB. And we need to calculate it too.

So I use webpack to get more accurate size. What do you think about it?

@siddharthkp
Copy link
Owner

Oh, I completely missed your point. Now, I get it.

Doesn't the size of the application bundle depend on the other dependencies present in the application well? (minification + gzip depends on all of the code)

Related: What would you include for a library like styled-components or react-redux which has a peer dependency on react

@siddharthkp
Copy link
Owner

siddharthkp commented Jul 3, 2017

How I assumed bundlesize would be used is:

  1. Library authors use it for their dist version.
  2. Application maintainers use it for their webpack/rollup/browserify outputs.

@ai
Copy link
Author

ai commented Jul 3, 2017

Related: What would you include for a library like styled-components or react-redux which has a peer dependency on react

Good question about peerDependencies. Maybe we should remove peer dependencies from bundle. I could try to update code. Great idea, I missed it.

Doesn't the size of the application bundle depend on the other dependencies present in the application well? (minification + gzip depends on all of the code)

Yeap, of course application bundle will not increase to exactly same number. So if bundlesize will show 10KB, application bundle could increase to 9KB, because of gzip tricks.

So I agree that this method is not 100% accurate. But my point, that using webpack will have better (not ideal, but better) result, that not using it.

Do I understand correctly that you share with me idea, that bundlesize should protect libraries to become very big? In my experience on Logux Client I found 2 most popular mistakes:

  1. Unexpected bug dependency in library.
  2. Wrong using of process, so webpack will add huge polyfill when I will add library to my app.

So using more accurate bundle size calculation will not be ideal, but it will be better and will prevent libraries from really bad mistakes.

@Furizaa
Copy link
Contributor

Furizaa commented Jul 3, 2017

Do I understand this correctly that webpack is used to pack the bundle on which the size is calculated? Our project I'm working on 9-to-5 is already bundled with webpack and the build takes almost 10 minutes. Using a webpack buid again just to determine the size would double that time.

This was the hook in this project. I can just point it to the bundles that are already there.

@ai
Copy link
Author

ai commented Jul 3, 2017

Library authors use it for their dist version.

This exactly what I assumed, when create Size Limit :D.

The only difference is that we can try to prevent more mistakes for same users. Library author could accidentally add 1MB dependency in small PR.

But “Application maintainers” is good thought. Yeap, they didn’t need extra webpack. Maybe we could add --no-pack option to them?

@ai
Copy link
Author

ai commented Jul 3, 2017

Also very important feature of using webpack is Webpack Bundle Analyzer. So you will be able to understand why your library has this size. It helps me a lot in Logux Client development (Size Limit was just a snippet for Logux Client).

Bundle Analyzer example

@siddharthkp
Copy link
Owner

This was the hook in this project. I can just point it to the bundles that are already there.

  1. I completely agree with this. I think we should stick with this approach. bundlesize should be agnostic to their build mechanism

  2. Library authors that want to go the extra mile, will test out a sample application with their library. @rauchg suggested doing the same for nextjs

  3. I do see your point @ai, these are useful additional features but I think it's slightly outside the scope of bundlesize. bundlesize solves a small problem well, I would want to keep it that way. Sorry!

@Furizaa
Copy link
Contributor

Furizaa commented Jul 3, 2017

I get what you mean. But I believe that even most libraries will provide one or more minified umd packages (e.g. react.min.js) that are the single source of truth regarding bundle size - the bytes this libraries deliver to the userland end the end.

@ai
Copy link
Author

ai commented Jul 3, 2017

@Furizaa

I get what you mean. But I believe that even most libraries will provide one or more minified umd packages (e.g. react.min.js) that are the single source of truth regarding bundle size - the bytes this libraries deliver to the userland end the end.

There is not such single bundle in PostCSS. Also without webpack library authors will not be able to see Webpack Bundle Analyzer.

@siddharthkp
Copy link
Owner

There is not such single bundle in PostCSS

I see how bundlesize completely fails in the case of tools (works for libraries/applications). However, my suggested solution would be to keep the webpack build inside postcss and test that with bundlesize, instead of expecting bundlesize to build it.

Also without webpack library authors will not be able to see Webpack Bundle Analyzer.

Not in the scope of bundlesize.

@ai
Copy link
Author

ai commented Jul 3, 2017

@siddharthkp

my suggested solution would be to keep the webpack build inside postcss and test that with bundlesize, instead of expecting bundlesize to build it.

It will be hard to have bundle script in every project. Especially, if it is many small projects.

Webpack config often is bigger than library itself :D.

@ai
Copy link
Author

ai commented Jul 3, 2017

@siddharthkp

Not in the scope of bundlesize.

Why Bundle Analyzer is not a scope? Does scope of bundlesize is to control libraries from become bigger. In this case you need not only warning, but also reasons why library become bigger.

Profiling is a key for optimization ;).

@siddharthkp
Copy link
Owner

siddharthkp commented Jul 3, 2017

In this case you need not only warning, but also reasons why library become bigger.

This is an interesting take, but I'm not sure if I like the solution. Let me think about this problem a little more. I'll create an issue for this.

It will be hard to have bundle script in every project. Especially, if it is many small projects.

Here's how I see it: Right now bundlesize is lazer focused on solving one problem, I'm afraid of adding more features that only help a subset of users/use cases.

According to me, it makes more sense to create another package which is like a setup step for bundlesize that is useful for these users.

@ai
Copy link
Author

ai commented Jul 3, 2017

According to me, it makes more sense to create another package which is like a setup step for bundlesize that is useful for these users.

Haha, it is not how open source works. Everyone ignored Size Limit in Twitter :(.

@siddharthkp
Copy link
Owner

@ai Sorry, I don't know how I can help 😢

@ai
Copy link
Author

ai commented Jul 3, 2017

This is an interesting take, but I'm not sure if I like the solution. Let me think about this problem a little more. I'll create an issue for this.

OK, I will wait for answer

@siddharthkp
Copy link
Owner

Raised #27, Closing this pull request.

If I am unable to come up with anything better, will use your approach 👍

@siddharthkp siddharthkp closed this Jul 3, 2017
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

Successfully merging this pull request may close these issues.

3 participants