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

feat: use ts build #970

Merged
merged 13 commits into from
Aug 21, 2024
Merged

feat: use ts build #970

merged 13 commits into from
Aug 21, 2024

Conversation

spaenleh
Copy link
Member

@spaenleh spaenleh commented Jul 30, 2024

In this PR I convert the package to be built with Typescript only and not vite library mode.

Motivation

While vite library mode is very convenient to create a bundled package, it uses single file mode by default which makes the library code unable to be tree-shaked.

If we expose the library with preserved module structure, the library should be tree-shakable and the consumers should be able to reduce the size of their bundle by not including unused code.

Caveats

In my test, after exposing the Typescript build, the size of the consumer did not really decrease.
Possible reasons for this is that since I had to use the named imports of @mui/material instead of the second level default exports The bundle was still larger. This issue seems to not be on the fix list for the MUI team, as seems like a design issue on their behalf. If we wanted to really reduce our bundle size we should look at another component library that has better support for bundle size optimisation and tree-shaking. If looking at the analysis from https://arethetypeswrong.github.io/?p=@mui/[email protected] the issue lies in the fact that the second level imports are maskaraded in CJS exports. So it will not play nicely with the current ts + nodenext setup.

Other changes

I took the opportunity to do a bit of summer cleaning too:

  • use as much lucide-icons as possible to replace the mui icons
  • use type instead of interface in all prop definitions
  • remove the usage of FC in favour of (props: Props) => JSX.Element
  • remove intermediate index.ts files in subfolders and directly export the components in the main index.ts

Questions

Should we use only named exports for components instead of having default exports that are then named ?

Downstream test PRs and bundle size comparison

Player:

graasp/graasp-player#825
2,312.19 kB -> 1,589.27 kB
31% shrink in bundle size

Builder

graasp/graasp-builder#1390
4,646.32 kB -> 4,379.26 kB
5% shrink in bundle size

Library

graasp/graasp-library#665
1.56Mb -> 1.45 Mb
7% shrink in bundle size

Analytics

graasp/graasp-analytics#419
2,257.06 kB -> 2,033.90 kB
9% shrink in bundle size

Account

graasp/client#315
1,660.81 kB -> 1,351.62 kB
18% shrink in bundle size

Auth

graasp/graasp-auth#442
1,599.91 kB -> 1,280.01 kB
19% shrink in bundle size

@spaenleh spaenleh changed the title Use ts build feat: use ts build Jul 31, 2024
@spaenleh spaenleh requested review from pyphilia and ReidyT July 31, 2024 06:21
@spaenleh spaenleh self-assigned this Jul 31, 2024
Copy link
Contributor

@ReidyT ReidyT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, LGTM! 👾

src/Card/FolderCard.tsx Show resolved Hide resolved
src/Header/Header.tsx Outdated Show resolved Hide resolved
src/CreativeCommons/icons/CCIconsProps.js Outdated Show resolved Hide resolved
src/itemLogin/utils.ts Outdated Show resolved Hide resolved
Copy link

@spaenleh spaenleh added this pull request to the merge queue Aug 21, 2024
Merged via the queue into main with commit 5f963f7 Aug 21, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants