search

cube-js / cube

šŸ“Š Cube ā€” Universal semantic layer platform for AI, BI, spreadsheets, and embedded analytics
https://cube.dev
Other
18.01k stars 1.78k forks source link

Reduce bundle size of @cubejs-client #960

Open mgebeily opened 4 years ago

mgebeily commented 4 years ago

Is your feature request related to a problem? Please describe. The cube.js client is an awesome way to interact with the server. However, it's rather large for a client side library, with the bulk of the size of it being comprised of dependencies, and half of which come from a single library (moment). There are also polyfills bundled with the library that could be deferred to the user's own browser support needs. This could lead to a large increase in the size of javascript apps that utilize the cube client.

Describe the solution you'd like There are some smaller alternatives to moment that provide the same functionality. dayjs is one of the most popular and is 5% the size. Most of the usage of rambda could be replaced with native ES6 and polyfilled by the user depending on the browsers they support. There may also be value in re-arranging some of the code to allow for tree shaking.

Describe alternatives you've considered The alternative to this would be to interact directly with the server through http. The cube client provides some really useful functionality and would be great to use if it was a little more lightweight. It seems that the core of the package has almost all of the logic and makes up a small percentage of the size.

Additional context Any moment replacement would have to be strenuously tested to make sure that it can parse the same date formats, as users can pass arbitrary strings to moment. Even then, all of the above suggestions would probably be considered breaking. I wanted to see if others have the same concerns and if there was interest in a pull request before I try to create one. Let me know if I completely missed the mark with the above.

I'm really looking forward to working with cube. Reporting has always been a pain point for me and this seems like an excellent solution. Thanks in advance!

vasilev-alex commented 4 years ago

hi @mgebeily! Thanks for posting it here. Yes, it's difficult to disagree with you on the bundle size.

We could've just replaced moment with moment-mini that comes without unnecessary locales but we also depend on moment-range. I guess we can try dayjs if we can find an alternative to moment-range.

As for ramda we should opt for smaller bundle size as well, and find more lightweight alternatives for the methods we can't simply replace.

And, of course, PRs are very welcomed!

mgebeily commented 4 years ago

Fantastic! Thanks so much for the follow up. This isn't an easy issue to reply to. I'll work on getting a PR available for your consideration. Thanks again!

vasilev-alex commented 4 years ago

Let's keep it open then and track all related discussions here

sveisvei commented 3 years ago

I would like to help with this, how would you like this approached? Ramda I think is small enough that is does not matter, moment on the other hand is very large.

Skjermbilde 2021-03-26 kl 15 45 55
vasilev-alex commented 3 years ago

hi @sveisvei! A PR would be really appreciated. We can think of moment alternatives, but it should be replaced very carefully, so we don't break anything.

sveisvei commented 3 years ago

I have not had time to do this yet. If someone attempts this - be my guest. I will work on this eventually, if not.

As part of discussing the solution, we could make it "injectable" - so we can keep moment by default. If a project uses moment, there is no extra size overhead anyhow.

mgebeily commented 3 years ago

Sorry for the delay. I took a first crack at removing moment and moment-range in the above PR. It was suspiciously straightforward. I ended up not using cube for my last project (though I'm really hoping to soon) so I'd really appreciate someone with domain knowledge dropping a review on it. I've left it as a draft until I get a better idea on areas of concern.

joealden commented 2 years ago

@vasilev-alex do you have any thoughts regarding the initial mention of included polyfills (in this case, cross-fetch, core-js and url-search-params-polyfill)? In my personal opinion, this should be left up to the user (but likely noted in the relevant piece of documentation), as for example, my project only runs in the browser and has evergreen targets, so doesn't need either cross-fetch or url-search-params-polyfill (and maybe some of core-js that might be getting bundled).

As an aside, the latest version of core-js supports polyfilling URLSearchParams, so in any case, that dependency likely isn't needed anymore.