amcharts / amcharts4

The most advanced amCharts charting library for JavaScript and TypeScript apps.
https://www.amcharts.com/
1.15k stars 322 forks source link

tree shaking does not work - bundle size is huge #82

Open oocx opened 6 years ago

oocx commented 6 years ago

steps to reproduce: build a simple Angular cli project that uses amcharts4, for exapmle the sample from node_modules\@amcharts\amcharts4\examples\angular\simple-line-chart

The output contains 2 MB pdfmake.js and 1,2 MB xlsx.js, and main.js is way too large.

I started migrating to amcharts4 because the version 4 overview page advertises tree shaking, and I hoped to reduce my bundle size. Instead, bundle size is much larger now.

Pauan commented 6 years ago

@oocx I tried building our simple-line-chart example, here are the file sizes I got:

main.js           12 KB
polyfills.js     222 KB
runtime.js         8 KB
styles.js         16 KB
vendor.js      5,787 KB
             = 6,045 KB

I then tried removing all of the amCharts-related stuff, and tried building it again, and these are the file sizes I got:

main.js           11 KB
polyfills.js     222 KB
runtime.js         6 KB
styles.js         16 KB
vendor.js      2,672 KB
             = 2,927 KB

That means that the amCharts code is 3,118 KB. That's quite a lot, but it's not unreasonable compared to many modern JS libraries (Angular by itself is 2,927 KB!) And of course we are looking into ways to reduce the filesizes.

As for the extra files (such as fabric.js, pdfmake.js, responsivedefaults.js, and xlsx.js), they are not loaded by default, they are loaded on-demand if you use the Export functionality, so if you don't use Export then they won't be loaded at all.

peluprvi commented 6 years ago

Here I followed the amCharts 3 Vue.js with Vue CLI 3 tutorial.

Since it is not using the Export functionality, why it loads using @vue/cli 3 (tested on 3.0.0-rc.3) and it also getting the extra files (pdfmake.js , xlsx.js, etc) even on production mode or build?

If it needs to be disabled, how I can do that?

I got this files after build it:

File                                      Size             Gzipped

dist\js\pdfmake-legacy.2c5519d5.js        1670.31 kb       820.01 k
dist\js\xlsx.5692312e.js                  642.42 kb        313.54 k
dist\js\fabric.cc5cb61d.js                282.76 kb        78.68 kb
dist\js\responsivedefaults.84b7f978.js    10.07 kb         2.83 kb

image

I may have missed something here. Please let me know what I have to config here and also include the step in your tutorial page.

oocx commented 6 years ago

Adding a simple line chart to my app should not add 3 MB to the bundle size. When you analyze the sample with webpack-bundle-analyzer, you can see that the bundle contains the code of all chart types and all of amcharts features. If tree shaking was working correctly, I would expect the bundle to only contain the code for the chart types I used. So for the sample, it should contain only the LineSeries chart.

I see that https://www.amcharts.com/v4/ does not mention tree shaking any more, so I guess it now works as advertised :). However, it would be great if AmCharts would consider adding real tree shaking support. I will probably just use another, less feature rich but smaller library like chart.js (only 37kb),, as I don't need 90% of the features and code that amcharts would add to my bundle.

Pauan commented 6 years ago

@oocx Tree shaking of classes is a very tricky subject. Because of the dynamic nature of JavaScript (and the output of transpilers like Babel and TypeScript), it is very difficult for tree shaking to work correctly.

This problem isn't specific to amCharts, it happens with all uses of classes.

Here are some examples of that:

https://github.com/webpack/webpack/issues/2899

https://github.com/webpack/webpack/issues/4080

https://github.com/mishoo/UglifyJS2/issues/1261

https://stackoverflow.com/questions/39065038/how-to-use-tree-shaking-for-classes-with-typescript-and-webpack

It's not really something we can "fix", since there's nothing wrong with our code, it's caused by limitations in the other tools (like UglifyJS and Webpack).

We would love to have great tree shaking support, but we cannot change the JavaScript language, and we cannot improve the tree shaking algorithms used by UglifyJS or Webpack.

Having said that, we completely agree that 3 MB is quite large for simple charts, and we will continue to investigate ways to improve the file size (through tree-shaking improvements, or other approaches).

Pauan commented 6 years ago

@peluprvi You're right, I just tested our Vue tutorial and it does load those files, which is incorrect. With our Angular tutorial it correctly does not load the files.

I don't have any experience with Vue, I'll ask the rest of our team to take a look.

Pauan commented 6 years ago

@oocx Aha! I figured out the reason for the large file sizes. The Angular CLI does not use minification when doing ng build. Instead you have to use ng build --prod.

Here are the new file sizes when using ng build --prod:

main.js        819 KB
polyfills.js    59 KB
runtime.js       2 KB
             = 880 KB

And here are the file sizes when not using amCharts:

main.js        153 KB
polyfills.js    59 KB
runtime.js       2 KB
             = 214 KB

As you can see, amCharts only adds a 666 KB overhead. That's still a lot, but certainly much better than 3 MB!

ailon commented 6 years ago

@peluprvi we are still investigating a proper way for webpack/vue not to use those files by default (unless needed) but as a quick workaround you can tap into the Vue CLI generated webpack config and tell it not to prefetch these files.

To do that, create a vue.config.js file in the root of your project and add this to it:

// vue.config.js
module.exports = {
  chainWebpack: config => {
    config
      .plugin('prefetch')
      .tap(args => {
        return [
          {
            rel: 'prefetch',
            include: 'asyncChunks',
            fileBlacklist: [
              /\.map$/,
              /pdfmake\.[^.]+\.js$/,
              /xlsx\.[^.]+\.js$/,
              /fabric[^.]*\.[^.]+\.js$/,
              /responsivedefaults\.[^.]+\.js$/,
            ]
          }
        ]
      })
  }
}

The files will still be generated to the output directory but they will not be loaded, which is the most important part, imo.

peluprvi commented 6 years ago

@Pauan thanks for checking!

@ailon thanks for your effort! Your workarround only works when production mode (vue-cli-service serve --mode=production). It also broken the build process when using --modern flag (vue-cli-service build --modern).

To make it work with the --modern flag, I changed it to:

// vue.config.js
module.exports = {
  chainWebpack: config => {
    if (config.plugins.store.get('prefetch')) {
      config
        .plugin('prefetch')
        .tap(args => {
          args[0].fileBlacklist = [
            /\.map$/,
            /pdfmake\.[^.]+\.js$/,
            /xlsx\.[^.]+\.js$/,
            /fabric[^.]*\.[^.]+\.js$/,
            /responsivedefaults\.[^.]+\.js$/
          ]
          return args
        })
    }
  }
}

I agree that in my case the most important is to not load the files, but including unused files to the build process also increases its time (~85% of my building time right now is for that files).

I'll keep waiting for a definitive and automatic solution.

Edit: Also remembering if the product uses a column chart, the app doesn't need to load other chart scripts.

Pauan commented 6 years ago

Although it's not a substitute for tree shaking, gzip compression makes a huge difference:

Angular with amCharts    = 235.09 KB
Angular without amCharts =  86.02 KB

As you can see, with gzip compression enabled, amCharts V4 only adds 149.07 KB.

peluprvi commented 6 years ago

I created a base that you can download and edit to test it https://codesandbox.io/s/828rxoq2lj https://github.com/peluprvi/vue-amcharts4-teste Maybe there is a way to use babel-plugin-transform-imports to avoid full import the lib.

peluprvi commented 6 years ago

For Vue, it is related to https://github.com/vuejs/vue-cli/issues/979 and documented there on https://cli.vuejs.org/guide/html-and-static-assets.html#prefetch

eliashdezr commented 5 years ago

Is there any optimization for Angular 2+?

I see in the docs that the way to import it on Angular is like this:

import * as am4core from "@amcharts/amcharts4/core";
import * as am4charts from "@amcharts/amcharts4/charts";

This would not allow tree shaking to work as expected I believe.

Shyam-Chen commented 5 years ago
- import * as am4core from '@amcharts/amcharts4/core';
- import * as am4charts from '@amcharts/amcharts4/charts';
+ import { useTheme, create, Scrollbar } from '@amcharts/amcharts4/core';
+ import { XYChart, DateAxis, ValueAxis, LineSeries, XYCursor } from '@amcharts/amcharts4/charts';
peluprvi commented 5 years ago

Even by importing this way, it is including pdfmake, xlsx and others in the bundle process (Vue environment), increasing the bundle process time:

image

fonziemedia commented 5 years ago

Hi guys,

Any updates on optmising the bundle size?

When you say that js chunks like 'pdfmake' are loaded on demand, do you mean when the end user requests them or if we request it in a script?

darlesson commented 5 years ago

For anybody checking this ticket, I also recommend checking #569.

Pauan commented 5 years ago

@fonziemedia When the user exports the chart (using the export menu in the upper-right), then it will download the files. Until then, the files aren't downloaded.

You shouldn't include them in <script> tags, since they'll be loaded automatically on-demand.

fonziemedia commented 5 years ago

That's great @Pauan / @darlesson

Thanks for clarifying!

delaaxe commented 5 years ago

Bundle sizes are also huge using create-react-app. Why not restructure the project on a "one file per chart type" basis in order to make tree shaking work?

Pauan commented 5 years ago

@delaaxe We already do that (you can see the source code here).

The issue is that Webpack will always include all files (even files that aren't used). This is mandated by the ES6 spec.

Tree shaking just doesn't work as well in JavaScript as it does in other languages, and there isn't much we can do about it.

onx2 commented 5 years ago

I'm evaluating a few options for charting packages and have really enjoyed testing amcharts! It is very efficient and easy to jump into and having pdfmake load on demand is a great idea 😄

I do agree bundle size can be issue though. I'm seeing ~178kb gzipped after removing all of the extras like pdfmake and xlsx, which is still bigger than I was hoping for.

I think chart.js has been working on treeshaking in the same way some people have suggested here (import SomChartType from 'package/path' and having only that chart type and its dependencies loaded). It might be worth checking out a few of the issues / PRs they've created to see if anything might be useful for the amcharts team.

References note that they have had this issue being tracked since May 5, 2016 https://github.com/chartjs/Chart.js/issues/4478 https://github.com/chartjs/Chart.js/issues/2466

A clever or perhaps unintentionally effective approach to reduce bundle size that I've noticed Highcharts has done is to split out Highcharts from Highstock. Idk if this type of splitting of the amcharts package is possible but as you can see in the image below, if I didn't need the highstock functionality it significantly reduces bundle size.

Together they are the same size as amcharts, but individually are about half with Highstocks being about 100kb gzipped and 78kb gzipped for highcharts.

The difference between the two, at a quick glance, seems to be the time series advanced navigation tools like scrolling, zooming and date picker etc... Maybe amcharts could have a diet / lite amcharts version without these features? 😆

Highcharts and Highstock -- v7.1.1 image

amcharts without accessories -- v4.3.14 image

justinhelmer commented 5 years ago

Related: https://github.com/amcharts/amcharts4/issues/1267

Pauan commented 5 years ago

@onx2 We originally split each chart into a separate file (so you'd load @amcharts/amcharts4/charts/serial for example).

However, after looking at the compiled output, core.js is 733 KB, and all of the charts put together is 245 KB.

The core.js is shared by all the charts, so it can't really be tree shaken.

So it didn't make any sense to split the charts, since it wouldn't save much KB.

justinhelmer commented 5 years ago

@Pauan - It depends on what you define as "much kB".

The core is massive, but I understand that is a larger effort to tackle. @martynasma mentioned the interwoven nature of dependencies in core in #1267.

However, all charts combined at 245kB is not something that should be shrugged at.

Here is a great article by @addyosmani regarding the cost of JS, and what your JS budget (in kB) should be: https://medium.com/@addyosmani/the-cost-of-javascript-in-2018-7d8950fbb5d4

In today's web world, customers expect things to be immediate. In most projects, I limit the overall bundle size of all vendor dependencies combined to just 30kB (gzipped). amCharts is nearly 1MB combined, and can only be reduced to 187kB with minification/obfuscation/compression. This is huge.

I would suggest taking a phased approach to addressing this problem. 187kB with all of the most modern compression/obfuscation algorithms should not be considered acceptable. If splitting the charts out into individual consumable files is low-hanging fruit, then I think that would be a great place to start. It wouldn't solve the bigger issue, but it would be a positive step in the right direction. As an example, it would be possible to shave off the majority of the 245kB you cited if a consumer just needed a pie chart. More interestingly, it would also inherently introduce the ability to load specific charts on-demand, using techniques such as lazy-loading and code splitting.

I think it is important for your team to continue to elevate this as an essential benchmark for success of amCharts, as it is with any web-based project. Performance is a metric for success in today's web world, and heavily correlates to customer engagement and conversion. This metric especially holds weight for websites that thrive on SEO, as performance results have a direct correlation to their page rank. These are very real-world problems with metrics to support this need.

As an example, I once worked for a company that had tools in place to track performance and its impact on the conversion funnel. At one particular point in time we did an evaluation that resulted in understanding that a quarter of a second improvement in key performance metrics resulted in a 1.3% increase in revenue. Due to the volume of the B2C, such a change would yield a six figure improvement in revenue per day. These are not small numbers.

I appreciate the consideration.

marlonmleite commented 4 years ago

Any update? This is very critical.

My project avoids as many dependencies to be as small as possible, but after adding amCharts it went down the drain, extra 2MB and 300% larger bundle.

I understand that tree shacking in JS doesn't work very well, but combined with Code Spliting and Lazy load this is fine, but everything goes down the drain when amCharts comes in.

eliashdezr commented 4 years ago

Absolutely, at this point highcharts seems better option if performance is a must. Been trying to integrate amcharts but it is not feasible due the exceed of bundle size.

b49015 commented 4 years ago

Hi @Pauan and @peluprvi,

I am seeing a very huge bundle size because of pdfmake and xlsx, which I am not using in my project. I am using vue-cli version3.10 and amcharts4.6.4 and I have tried this workaround as well:

// vue.config.js module.exports = { chainWebpack: config => { if (config.plugins.store.get('prefetch')) { config .plugin('prefetch') .tap(args => { args[0].fileBlacklist = [ /.map$/, /pdfmake.[^.]+.js$/, /xlsx.[^.]+.js$/, /fabric[^.]*.[^.]+.js$/, /responsivedefaults.[^.]+.js$/ ] return args }) } } } in vue.config.js but it seems this is not working for me. here is the snapshot: amcharts_log

Is anybody working on this issue? Can someone point me what am I doing wrong?

justinhelmer commented 4 years ago

I would love to get some traction on this issue. The impact is debilitating.

As you can see in @b49015 's post above, using webpack-bundle-analyzer illustrates the issue well. In the above image, the orange, green, purple and cyan blocks are all dependencies of amcharts. It's 80% of the entire size of the project for a single library. As you can see, all other third party dependencies combined (node_modules) are smaller than the single core charts.js file.

When using webpack-bundle-analyzer with our project, we get very similar results.

At the price point of amcharts, there should be an expectation of a higher standard for performance.

Please revisit this as a priority.

delaaxe commented 4 years ago

@b49015 @justinhelmer When doing a production build, pdfmake.js and xlsx.js are compiled into their own bundles which never get downloaded by the browser if you don't use the PDF or Excel export features.

The core amcharts code is still huge and should be made more modular. ES modules anyone?

Pauan commented 4 years ago

@justinhelmer 30KB is an awfully low limit. Angular is 6MB, jQuery is 86KB, React is 128KB, PixiJS is 359KB, Highcharts is 233KB. All are widely used and accepted by organizations.

Obviously we want smaller code size, but that has to be balanced with providing value to our customers, since our customers expect a lot of functionality. We provide more features than any other charting library.

The idea of splitting each chart type into a separate file is something we want to try, but it's tricky to get the build process right.


@marlonmleite You shouldn't be seeing a 2MB increase. Are you excluding pdfmake and xlsx from the size calculations?


@delaaxe We already use ES6 modules. The problem is that dead code elimination does not work very well for JS classes. We've considered various ways to workaround that, but it's a very tricky topic, which is why other projects (such as Angular) also struggle with large file sizes, it's definitely not unique to amCharts.

develar commented 4 years ago

@Pauan Currently, code of amcharts organized in a way, that even if you prefer to import only used functionality, you cannot.

Code (import { XYChart, DateAxis } instead of * as am4charts) is not possible, because required classes under ./.internal. internal it is clear indicator, that you don't want to expose it, right?

Question is — do you still insist that import * it is the only way to use amcharts? Or the only reason why it is so, it is concern about compatibility, — you don't want to keep backward compatibility on file name level.

justinhelmer commented 4 years ago

@develar - The issue is more complex than that, as it takes rewriting some of the fundamental infrastructure of the product. Tree shaking is a feature separate from ESM; it is controlled by the bundler/compiler. You will find that even if you change the syntax (i.e. import { XYChart } from amcharts), this will have no impact on the final bundle size.

More on the topic:

For those seeking easy answers, please do some research on the concept of "pure modules" and "dead code elimination" as it relates to tree shaking. You will then understand the complexity of what we are asking from the amcharts team.

justinhelmer commented 4 years ago

@Pauan - I appreciate you taking the time to respond and move the needle on this topic.

I’d like to correct some misinformation about library sizes. We should be talking about minified & compressed versions. Otherwise, it would be only fair to compare to the unminified and uncompressed version of amcharts, which is nearly 1MB (733kB just for the core, without charts).

To provide more accurate data, here are the actual compressed/minified sizes for the tools you mentioned in your most recend comment (showing latest stable version of each):

Compare this against just the core + charts of amcharts (not including pdfmake, xlsx etc.) which comes to 245Kb (as referenced in my original post. This is greater than 3x even the largest library from the list you mentioned.

I would also like to humbly disagree with you that 30kB is an “awfully low limit”. That leads me to believe you did not have the opportunity to read the great article I referenced in my original post about the cost of JavaScript, or you otherwise disagree with @addyosmani on this. With all due respect, he is leading the industry in these types of investigations. Additionally, I provided you with some real-world metrics and data as it related to actual financial cost and opportunity. I figured this would sway those on the fence about the importance of this topic.

I understand the complexity of this ask, as I have had to personally lead an effort on something very similar in the past on a large-scale product. I am only hoping to elevate the importance of this topic within your organization, and help you see the opportunity cost in the same way as the growing number of your customers.

develar commented 4 years ago

Actually, I do not expect that JS will be ever to provide something like -ldflags='-s -w' and that it will be fully working.

My point is — if you use amcharts with webpack, you don't use compiled version of library. Webpack compiles it for you using referenced files. So, possible solution is not related to tree shaking at all.

If you will include required functionality directly from corresponding files, then unneeded functionality will be not included into compilation at all (since not imported). So, without relying on tree shaking, you will get desired result.

Is it hard?

From developer (client of amchart) standpoint: if you use modern IDE, then you don't worry about importing at all, since you simply type in XY... and IDE will complete statement, insert import or modify existing import statement.

From amcharts author standpoint:

In any case, I am fully satisfied with current state and don't think that 209KB gzipped it is a real problem (http2, prefetch and so on). Just I don't see any reason why not to allow import of required functionality directly without any facade.

delaaxe commented 4 years ago

Doesn’t tree shaking “just work” when the project is organized on a “one file per class/function” basis?

Sent with GitHawk

justinhelmer commented 4 years ago

Why was this closed? I laid out in detail the problem, empirical information to show why it is a problem, provided multiple solutions, and a path of least friction to resolving this, or at least moving the needle.

It is incredibly frustrating that this issue was just closed without any reasoning or addressing my comments or concerns in any way.

I seriously hope your team reconsiders the value of bundle size as it relates to performance in the modern web with respect to large-scale applications with many users.

Good day.

eliashdezr commented 4 years ago

@martynasma yes, I´m not convinced that this should be closed, as it is not fixed. A better tag should be "WONT FIX".

Pretty annoying by this decision.

martynasma commented 4 years ago

Well, this is what happens when you try to batch-close "stale" threads. Inadvertently you shoot a live one. Sorry guys. Reopening.

jimmykane commented 4 years ago

Ok I am not an expert here, but instead of fighting packers and tree shaking issues, could it be a solution to introduce a breaking change that PDF becomes a plugin and separate package?

Or am I so out of scope in this?

justinhelmer commented 4 years ago

@jimmykane - pdfmake (and xlsx) are both loaded on-demand already, so are not really in consideration as part of the root of the issue. It is about the core + charts. See my earlier comment.

angelnikolov commented 4 years ago

Any movement on this one? I've sucessfully removed the pdfmake, xlsx and etc by using:

  externals: function (context, request, callback) {
    if (/xlsx|canvg|pdfmake/.test(request)) {
      return callback(null, "commonjs " + request);
    }
    callback();
  }

However as @justinhelmer pointed out a couple of times, the problem still remains that all of the charts are bundled in the built project, no matter what you do. This is an example partition from a bundle analysis I did on our project. image

We don't use any of those series like Pyramid, Radar, TreeMap and etc.. I don't even want to look at what core imports. We are a paying customer of Amcharts and we love the library and its capabilities. However, we are not only paying for the license but also for the increasing cost of building and distributing our project which the heavy bundle size only adds up to. cc @martynasma

BdDragos commented 4 years ago

@angelnikolov Hello. Will that function work in Angular 9 also, and if yes where should it be put? Also if I am already using pdfmake as a project dependency, will that piece of code also remove it or will it only remove the pdfmake from amcharts4?

delaaxe commented 4 years ago

This is how react-dnd declared their packages side effect free in order to enable tree shaking.

Seemed pretty easy for them : https://github.com/react-dnd/react-dnd/pull/1577

Sent with GitHawk

e-simo commented 4 years ago

Hey guys. You can use lazy loading using the instructions in this link: https://www.amcharts.com/docs/v4/tutorials/lazy-loading-amcharts-modules/ Divides in several chunks and significantly reduces the size of your main bundle. I have also observed performance improvements.

souljorje commented 3 years ago

This helped me to save some kilobytes, based on this article.

  1. Import separate modules to single file
    // ./plugins/amcharts.js
    export {create} from "@amcharts/amcharts4/.internal/core/utils/Instance";
    export {percent} from "@amcharts/amcharts4/.internal/core/utils/Percent";
    export {color} from "@amcharts/amcharts4/.internal/core/utils/Color";
    export { MapChart } from "@amcharts/amcharts4/.internal/charts/types/MapChart";
    export {Mercator} from "@amcharts/amcharts4/.internal/charts/map/projections";
    export { MapPolygonSeries } from "@amcharts/amcharts4/.internal/charts/map/MapPolygonSeries";
    export { HeatLegend } from "@amcharts/amcharts4/.internal/charts/elements/HeatLegend";
  2. Use lazy loading
    const amcharts = await import('@/plugins/amcharts');
    const worldLow = await import("@amcharts/amcharts4-geodata/worldLow").then((m) => m.default);
    const chart = amcharts.create('chart', amcharts.MapChart);
sassomedia commented 3 years ago

@martynasma just to check in, is this still a work in progress or potential milestone?

march08 commented 3 years ago

Same question here, we are close to 3 years of opening this issue :))

andreiwrk commented 2 years ago

If anybody is interested in how to remove pdfmake and xlsx using CRACO:

// in craco.config.js

module.exports = {
    webpack: {
        configure: ( webpackConfig, {env, paths} ) => {
            webpackConfig.externals = [
                ( context, request, callback ) => {

                    if (/pdfmake|xlsx/.test( request )) {
                        return callback( null, 'commonjs ' + request )
                    }

                    callback()
                }
            ]

            return webpackConfig
        }
    }
}