-
Notifications
You must be signed in to change notification settings - Fork 322
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
tree shaking does not work - bundle size is huge #82
Comments
@oocx I tried building our
I then tried removing all of the amCharts-related stuff, and tried building it again, and these are the file sizes I got:
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 |
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 ( If it needs to be disabled, how I can do that? I got this files after build it:
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. |
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. |
@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: 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). |
@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. |
@oocx Aha! I figured out the reason for the large file sizes. The Angular CLI does not use minification when doing Here are the new file sizes when using
And here are the file sizes when not using amCharts:
As you can see, amCharts only adds a 666 KB overhead. That's still a lot, but certainly much better than 3 MB! |
@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
The files will still be generated to the output directory but they will not be loaded, which is the most important part, imo. |
@Pauan thanks for checking! @ailon thanks for your effort! Your workarround only works when production mode ( To make it work with the // 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. |
Although it's not a substitute for tree shaking, gzip compression makes a huge difference:
As you can see, with gzip compression enabled, amCharts V4 only adds |
I created a base that you can download and edit to test it |
For Vue, it is related to vuejs/vue-cli#979 and documented there on https://cli.vuejs.org/guide/html-and-static-assets.html#prefetch |
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. |
- 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'; |
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? |
For anybody checking this ticket, I also recommend checking #569. |
@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 |
That's great @Pauan / @darlesson Thanks for clarifying! |
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? |
@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. |
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 ( References note that they have had this issue being tracked since May 5, 2016 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? 😆 |
Related: #1267 |
@onx2 We originally split each chart into a separate file (so you'd load However, after looking at the compiled output, The So it didn't make any sense to split the charts, since it wouldn't save much KB. |
@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 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 I would suggest taking a phased approach to addressing this problem. 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. |
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. |
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. |
I would love to get some traction on this issue. The impact is debilitating. As you can see in @b49015 's post above, using When using At the price point of Please revisit this as a priority. |
@b49015 @justinhelmer The core amcharts code is still huge and should be made more modular. ES modules anyone? |
@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 @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. |
@Pauan Currently, code of amcharts organized in a way, that even if you prefer to import only used functionality, you cannot. Code ( Question is — do you still insist that |
@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. 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 |
@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 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 I would also like to humbly disagree with you that 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. |
Actually, I do not expect that JS will be ever to provide something like 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 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. |
Doesn’t tree shaking “just work” when the project is organized on a “one file per class/function” basis? Sent with GitHawk |
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. |
@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. |
Well, this is what happens when you try to batch-close "stale" threads. Inadvertently you shoot a live one. Sorry guys. Reopening. |
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? |
@jimmykane - |
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. We don't use any of those series like |
@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? |
This is how react-dnd declared their packages side effect free in order to enable tree shaking. Seemed pretty easy for them : react-dnd/react-dnd#1577 Sent with GitHawk |
Hey guys. You can use lazy loading using the instructions in this link: |
This helped me to save some kilobytes, based on this article.
// ./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";
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); |
@martynasma just to check in, is this still a work in progress or potential milestone? |
Same question here, we are close to 3 years of opening this issue :)) |
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
}
}
} |
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.
The text was updated successfully, but these errors were encountered: