amcharts / amcharts5

The newest, fastest, and most advanced amCharts charting library for JavaScript and TypeScript apps.
Other
353 stars 94 forks source link

ES6 Modules #294

Closed bestickley closed 2 years ago

bestickley commented 2 years ago

Hi, thank you this excellent library. I'd like to request ES6 modules so improve bundle size. You can see here (scroll down) there are no ES6 module exports. Thank you!

Pauan commented 2 years ago

@bestickley amCharts 5 is completely 100% ES6 modules, and most of our dependencies use ES6 modules as well.

It seems that tool isn't doing a good job of detecting our code, we'll look into it.

Pauan commented 2 years ago

We made some changes which should help BundlePhobia to detect our ES6 modules. The changes will be in the next release.

bestickley commented 2 years ago

@Pauan, I apologize for misunderstanding. Glad to hear that. Let me ask a follow up question: why in the examples are default imports used instead of named imports? Default import: import am5 from "@amcharts/amcharts5"; Named import: import { Root } from "@amcharts/amcharts5"; My understanding (could be wrong) is that named imports allow tree-shaking of the unused exports from "@amcharts/amcharts5" but a default import will import all modules exported from "@amcharts/amcharts5" regardless of if they're used. I use named imports for the charts I'm building but I would think it would be helpful for future users to have named imports in the examples.

Also, when looking inside my node_modules at amcharts5 I noticed an examples folder, is that supposed to be installed? I would think not.

martynasma commented 2 years ago

Fixed in 5.1.5.

[5.1.5] - 2022-02-22

Added

Changed

Fixed

Full change log.

Download options.

Make sure you clear your browser cache after upgrading. And feel free to contact us again if you are still experiencing this issue.

coderbyheart commented 2 years ago

That change breaks bundling the library with Vite.js:

[vite:resolve] Missing "./xy" export in "@amcharts/amcharts5" package
error during build:
Error: Missing "./xy" export in "@amcharts/amcharts5" package
import * as am5xy from '@amcharts/amcharts5/xy'

Works fine with 5.1.4

nseb commented 2 years ago

Same problem

martynasma commented 2 years ago

We're reverting and reopening this, as the fix was causing other issues. Will need to revisit.

Pauan commented 2 years ago

@bestickley My understanding (could be wrong) is that named imports allow tree-shaking of the unused exports from "@amcharts/amcharts5" but a default import will import all modules exported from "@amcharts/amcharts5" regardless of if they're used.

If you use import * as am5 from "@amcharts/amcharts5"; then Webpack is smart enough to tree shake it.

Of course you can use named imports if you're more comfortable with it, but it shouldn't affect the tree shaking. You can verify this yourself by compiling your code with and without named imports and checking the size.

bestickley commented 2 years ago

Got it, thanks for clarifying @Pauan. I use Vite, but tree-shaking principles should still apply. Feel free to close!

Pauan commented 2 years ago

@bestickley Rollup is also smart enough to do tree shaking when importing the entire module. Internally Vite uses Rollup, so it should be okay.

In our next release we fixed the issue with BundlePhobia (hopefully properly this time).

bestickley commented 2 years ago

Got it. Thank you for explaining @Pauan and excellent library!

martynasma commented 2 years ago

Fixed in 5.1.8.

[5.1.8] - 2022-03-10

Added

Changed

Fixed

Full change log.

Download options.

Make sure you clear your browser cache after upgrading. And feel free to contact us again if you are still experiencing this issue.