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

fix: inline icons from @mui/icons-material #147

Merged
merged 2 commits into from
Jan 14, 2023

Conversation

rtritto
Copy link
Contributor

@rtritto rtritto commented Jan 11, 2023

Changes:

  • Remove @mui/icons-material dependency

  • Create Icons with svg

Related #140

@netlify
Copy link

netlify bot commented Jan 11, 2023

Deploy Preview for any-viewer ready!

Name Link
🔨 Latest commit 79be0ef
🔍 Latest deploy log https://app.netlify.com/sites/any-viewer/deploys/63c116f00efeeb0008f02dc0
😎 Deploy Preview https://deploy-preview-147--any-viewer.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@pionxzh
Copy link
Collaborator

pionxzh commented Jan 11, 2023

Thanks for the PR 👍
Will it be better that use deep import to reduce the bundle size? (Just curious)
Like:

-import { Check as CheckIcon } from '@mui/icons-material'
+import CheckIcon from '@mui/icons-material/Check'

@rtritto rtritto changed the title Remove @mui/icons-material Remove @mui/icons-material dependency Jan 11, 2023
@rtritto
Copy link
Contributor Author

rtritto commented Jan 11, 2023

As alternative, create a single Icons file to include all icons?

@rtritto
Copy link
Contributor Author

rtritto commented Jan 11, 2023

@pionxzh changed b276acd

@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

Base: 87.92% // Head: 87.78% // Decreases project coverage by -0.13% ⚠️

Coverage data is based on head (79be0ef) compared to base (4d6a858).
Patch coverage: 87.27% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #147      +/-   ##
==========================================
- Coverage   87.92%   87.78%   -0.14%     
==========================================
  Files          18       18              
  Lines        1904     1940      +36     
  Branches      339      345       +6     
==========================================
+ Hits         1674     1703      +29     
- Misses        230      237       +7     
Impacted Files Coverage Δ
src/components/Icons.tsx 84.78% <84.78%> (ø)
src/components/DataKeyPair.tsx 71.69% <100.00%> (ø)
src/components/DataTypes/Object.tsx 86.98% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pionxzh
Copy link
Collaborator

pionxzh commented Jan 12, 2023

I just checked the bundle size of this PR, but it didn't bring down the total size 😢
We might need some other way to fix it.
Btw, I want to set up some bundle size checker to make developers aware of the size diff.

@rtritto
Copy link
Contributor Author

rtritto commented Jan 12, 2023

Anyway this PR removes 1 dependency, can be merged

@rtritto rtritto force-pushed the remove-mui-icons-material branch from b276acd to 79be0ef Compare January 13, 2023 08:31
@rtritto
Copy link
Contributor Author

rtritto commented Jan 13, 2023

@pionxzh rebased

@anthonyalayo
Copy link
Contributor

anthonyalayo commented Jan 13, 2023

I was analyzing why so many modules are being used for my next.js dev build, and it pointed to the fact that json-viewer uses icons-material. Could we give this a merge? That would be great!

@pionxzh
Copy link
Collaborator

pionxzh commented Jan 14, 2023

Let's ship this. Thanks for your work @rtritto

@pionxzh pionxzh changed the title Remove @mui/icons-material dependency fix: inline icons from @mui/icons-material Jan 14, 2023
@pionxzh pionxzh merged commit 84a5d06 into TexteaInc:main Jan 14, 2023
@pionxzh
Copy link
Collaborator

pionxzh commented Jan 14, 2023

@anthonyalayo May I know your setup? bundler and the config.
Please let me know if it gets improved in v2.13.0 🙏

@anthonyalayo
Copy link
Contributor

anthonyalayo commented Jan 14, 2023

Hey @pionxzh, thanks for the release! I took a while on the response as I was double checking all the numbers.

I'm playing with a next.js boilerplate with version 13.1.2.

Here's what the compile output looks like with:

  1. next dev
  2. "@textea/json-viewer": "2.12.5"
  3. outputFileTracing: true and output: 'standalone' in next.config.js:

compiled client and server successfully in 38.3s (12492 modules)

Notice the extremely large modules count and long build time (its basically an empty project).
The next configuration above outputs a .next/trace which gives more details:

image

As you can see, there was a ton of hits from @mui/icons. I took a look in the package-lock.json to see that only @textea/json-viewer was using it.

Here's what the compile output looks like with:

  1. next dev
  2. "@textea/json-viewer": "2.13.0"
  3. outputFileTracing: true and output: 'standalone' in next.config.js:

event - compiled client and server successfully in 17.2s (1936 modules)

Much better! I can see that the 10K+ @mui/icons dependencies are gone:

image

With that being said, the dependency on @mui/material is still causing a lot of bloat:

image

Fixing that would probably take the module count down to something reasonable.

MUI has a page dedicated to techniques ensuring it doesn't bloat builds.
Would applying that to your MUI components be possible?

@pionxzh
Copy link
Collaborator

pionxzh commented Jan 14, 2023

@anthonyalayo You are right. I just checked the output and yes it didn't get transformed properly.

@rtritto rtritto deleted the remove-mui-icons-material branch January 14, 2023 13:48
@anthonyalayo
Copy link
Contributor

anthonyalayo commented Jan 15, 2023

@pionxzh @rtritto I made a PR for a fix, could you review it here?

Yazawazi pushed a commit that referenced this pull request Mar 10, 2023
* Remove @mui/icons-material

* Merge all icons to Icons file
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 this pull request may close these issues.

3 participants