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

Unnecessary code gets bundled when adding a component #3

Closed
kubk opened this issue Mar 11, 2024 · 9 comments · Fixed by #6
Closed

Unnecessary code gets bundled when adding a component #3

kubk opened this issue Mar 11, 2024 · 9 comments · Fixed by #6

Comments

@kubk
Copy link

kubk commented Mar 11, 2024

Hi. The library has a lot of working components, which is great because users can already start integrating them into their projects. The issue I noticed is that even importing one component results in a lot of unnecessary bundled code, causing the bundle to bloat.

Here is my code example:

<AppRoot>
  <SegmentedControl>
    {labels.map(({ value, label }) => (
      <SegmentedControl.Item
        key={value}
        selected={selected === value}
        onClick={() => setSelected(value)}
      >
        {label}
      </SegmentedControl.Item>
    ))}
  </SegmentedControl>
</AppRoot>;

It produces the following report generated by the vite-bundle-visualizer:

Screenshot 2024-03-11 at 09 47 14

Here we see a lot of tgui components (all the components I guess), Radix UI, a library to remove scroll. Reproduction repo: https://github.com/kubk/tgui-bundle-size-issue

Why tree-shaking is important
Users may be reluctant to use the library because importing even one component results in loading a lot of unnecessary code.

@mainsmirnov
Copy link
Collaborator

It seems like your Vite configuration might not be set up correctly. Our library supports tree shaking, and its bundle size is quite minimal. You can verify its size on BundlePhobia.

Additionally, you can test tree shaking with an appropriate build tool, such as react-scripts, which you can find here: react-scripts on npm.

Alternatively, consider using our template available at XeleneStudio TGUI-Example on GitHub

@kubk
Copy link
Author

kubk commented Mar 11, 2024

@mainsmirnov It's default Vite configuration. You can recreate it via npm create vite@latest my-react-app -- --template react-ts and then add tgui.

vite has 11.7 millions downloads weekly: https://www.npmjs.com/package/vite
react-scripts has 3.2 millions: https://www.npmjs.com/package/react-scripts
It's even doubtful whether react-scripts is still maintained: facebook/create-react-app#13503

Vite works fine with code splitting out of the box, the code splitting works for my telegram mini app and other dependencies, except tgui. I'd consider this worth looking from the tgui side, if the library doesn't want to miss plethora of vite users.

@kubk
Copy link
Author

kubk commented Mar 11, 2024

Not sure if tgui is going to be included in the Telegram Mini App template that is being built by @heyqbnk, but that template is also based on Vite: https://github.com/Telegram-Mini-Apps/reactjs-template.

@heyqbnk
Copy link
Member

heyqbnk commented Mar 11, 2024

Hey! Yeah, I have plans on adding this package to the templates based on React. I tried to use it several days ago, but met some problems, and decided to postpone the idea of integrating it. Moreover, it seems like the package is hardly bound to the Telegram SDK and I am not sure if it can be used along with @tma.js.

Thanks for your mention, because I forgot to check the bundle size with this package included. React JS template already uses @tonconnect/ui-react (~300kb) and react-router-dom (~200kb). Adding 1 more huge library will just blow my mind. So, we are not going not add this package until its size is optimal and problems are solved.

Waiting for grant to be completed and coming back to see the results. I am currently developing the separate package working with Telegram SDK and @tma.js using Solid.

@mainsmirnov mainsmirnov reopened this Mar 11, 2024
@heyqbnk
Copy link
Member

heyqbnk commented Mar 11, 2024

Due to the reason, that bundlephobia is currrently down, I have tried bundlejs.com to check the bundle size.

image

As you can see here, using the only Button component costs 226 kb.

Not sure that config could be the problem, because we use default config that already includes tree shaking and works fine with other package

@mainsmirnov
Copy link
Collaborator

mainsmirnov commented Mar 11, 2024

We adopted the build configurations from the library. If the issue also exists there, it might be necessary to adjust your setup.

My attempt to replicate the issue with webpack was unsuccessful. Checking the library's bundle reveals that files are individually compiled, ensuring full tree-shaking support.

We will take a closer look look at it, thank you for your notes and adjustments

@heyqbnk
Copy link
Member

heyqbnk commented Mar 11, 2024

I have tried this code locally:

import { Typography } from '@xelene/tgui';

console.log(Typography);

After building the code using Vite (which uses Rollup, minifying the code via terser), I receive 204kb bundle size. In case, I import only React, the bundle size becomes 142kb.

I have tried this code to see if we pull whole package importing the only Typography component:

import * as tgui from '@xelene/tgui';

console.log(tgui);

Bundle size is 295kb. So, we don't pull the whole package importing the only one component, but half of it.

Then, I tried to import this component using this way:

import { Typography } from '@xelene/tgui/dist/components/Typography';

console.log(Typography);

Bundle size is now 10kb.

So, I assume, that package contains side-effects which can't be tree-shaken. Importing any component from the root will lead to importing all side-effects. That's why importing Typography not from the root costs way less bundle size.

Let me know if I am missing something.

@mainsmirnov
Copy link
Collaborator

Thank you for providing the detailed repository with a demonstration. We have resolved the issue in version 2.0.4

Screenshot 2024-03-13 at 12 52 30 PM

@kubk
Copy link
Author

kubk commented Mar 13, 2024

@mainsmirnov Thank you! Can confirm it fixed the issue 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants