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

Provide unminified ES5 code as a target #429

Closed
adrm opened this issue Nov 16, 2017 · 11 comments
Closed

Provide unminified ES5 code as a target #429

adrm opened this issue Nov 16, 2017 · 11 comments
Assignees
Labels
rc3.0 Required Issue for the upcoming Version 3 status:completed Finished feature or fix status:in_progress Issue that is being worked on right now type:feature A new feature request type:maintenance A regular maintenance chore or task, including refactors, build system and performance improvements

Comments

@adrm
Copy link
Contributor

adrm commented Nov 16, 2017

Expected Behavior

The bundle size should be as small as possible.

Current Behavior

Although britecharts works fine with webpack and it only bundles the required charts as expected, right now every chart imported is weighting more than 100KiB parsed and around 45KiB gzipped, which I think it's too much.

Possible Solution

I don't know if I am doing something wrong, but I can see that webpack imports from dist/umd so maybe the imports are already bundling multiple dependencies that could be shared across charts.

@Golodhros
Copy link
Collaborator

Hi @adrm , thanks for creating an issue!

Totally agree, the bundle size should be as small as possible. Right now, when you load one of our charts, its bundle contains all the D3 modules that chart needs (you will only d3-selection to make it work).

Other users have voiced their concerns and it seems that we will be working on a new build target that would be unminified ES5 code in UMD or CommonJS format, so you can use your Webpack to create a bundle and avoid repeated dependencies.

Would that work for your use case?

@Golodhros Golodhros added the type:question An issue or PR that needs more information or a user question label Nov 16, 2017
@adrm
Copy link
Contributor Author

adrm commented Nov 16, 2017

Yes! That would be perfect. That would allow webpack to optimize the bundle and avoid the duplicated dependencies. The bloat was a big concern when choosing the library, so I hope you can fix this soon.

Thanks a lot for your work, it's working perfectly.

@Golodhros Golodhros changed the title Britecharts bundle size is too big Provide unminified ES5 code as a target Dec 22, 2017
@Golodhros Golodhros added type:maintenance A regular maintenance chore or task, including refactors, build system and performance improvements type:feature A new feature request help wanted Indicates we are looking for contributors on this issue proposal and removed type:question An issue or PR that needs more information or a user question labels Dec 22, 2017
@miglesiasEB
Copy link
Contributor

Looking into this it seems there isn't a way of doing it through webpack: webpack/webpack#2933

However, there seems to be a babel based solution commented in this response: https://stackoverflow.com/questions/41289200/output-an-es-module-using-webpack?answertab=votes#tab-top that could be useful.

I don't have a lot of background on babel, so any kind of help would be appreciated.

@wyaeld
Copy link

wyaeld commented Jun 13, 2018

So the @Golodhros solution works fine for britecharts-react itself, if you import modules using the lib/esm path it gets tree-shaken with webpack and results in minimal size.

Is there preventing this same technique from being applied to britecharts core? It appears to be a hefty 380kb zipped, which I expect is a lot of the d3, but we use D3 elsewhere too and import it in ways that webpack can tree-shake

Update: something must be misconfigured, because I can see that the brightcharts-react components already import from the minified umd of brightcharts, but just importing 2 charts isn't preventing the entirety of brightcharts coming in, and those individual .min files are rather large

@Golodhros
Copy link
Collaborator

Hi @wyaeld, thanks for your comment!

So, we definitely want to do this in Britecharts too. I will be super helpful. The first roadblock we have is that we need to transform our current AMD modules into ES modules. Once we do that, we should be able to apply the same setup we have in Britecharts-React and allow tree-shaking.

Regarding the inclusion of D3 within Britecharts, I had that question in my head for a long time. Should we pack the necessary pieces of D3 with our charts or should we allow users to use their own D3.js? At first, we favored the 'package' side of it, making it something self-contained (the whole library and the individual charts have the D3 parts they need, but they require the user to have d3-selection), but now we see that maybe that's not the best approach.

We could also proxy d3-selection to have it all there (it's already in the chart), or we could make d3 a dependency for Britecharts and hope that the users have the build tool configured to only use the needed modules.

What do you think?

@exequiel09
Copy link

What's the status of this issue?

@Golodhros
Copy link
Collaborator

Not much movement I am afraid.

As commented before, we need to first move Britecharts to use ES6 modules, then we could easily add the ES modules as a target as we do in Britecharts-React.

I would need help with the first task.

@Golodhros
Copy link
Collaborator

@adrm
Copy link
Contributor Author

adrm commented Jun 11, 2019

Maybe this issue can be closed in favor of #714 ? I understand that the problems that motivated this issue would be solved by distributing ES modules.

@Golodhros
Copy link
Collaborator

Well, they are closely related but not exactly the same. We can keep both.

@Golodhros Golodhros removed help wanted Indicates we are looking for contributors on this issue proposal labels Jul 11, 2019
@Golodhros Golodhros added this to In progress in Improve Developer Experience Aug 12, 2019
@Golodhros Golodhros added this to Backlog in Release 3.0 Dec 12, 2019
@Golodhros Golodhros added status:in_progress Issue that is being worked on right now rc3.0 Required Issue for the upcoming Version 3 and removed Status: Ready to Go labels Jun 15, 2020
@Golodhros Golodhros self-assigned this Jun 15, 2020
@Golodhros Golodhros moved this from Backlog to on Progress in Release 3.0 Nov 3, 2020
@Golodhros Golodhros mentioned this issue Nov 3, 2020
@Golodhros
Copy link
Collaborator

Done as part of V3 development.

Try this installing britecharts@next

@Golodhros Golodhros moved this from on Progress to Done in Release 3.0 Apr 9, 2021
@Golodhros Golodhros moved this from In progress to Done in Improve Developer Experience Oct 18, 2021
@Golodhros Golodhros added the status:completed Finished feature or fix label Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rc3.0 Required Issue for the upcoming Version 3 status:completed Finished feature or fix status:in_progress Issue that is being worked on right now type:feature A new feature request type:maintenance A regular maintenance chore or task, including refactors, build system and performance improvements
Development

No branches or pull requests

5 participants