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

Pull icons from CDN and remove from bundle #5865

Merged
merged 6 commits into from Jun 7, 2023

Conversation

johnnymetz
Copy link
Collaborator

@johnnymetz johnnymetz commented Jun 6, 2023

What does this PR do?

  • Closes https://github.com/pixiebrix/pixiebrix-app/issues/3475
  • Remove simple-icons and bootstrap-icons from bundle. This reduced the final bundle size from 19.8 MB to 16.9 MB.
  • Serve icons from CDN
  • Note, we still include the icon file names (e.g. 1password.svg) in the bundle. We need this so users can see / search icons.

Discussion

Demo

https://www.loom.com/share/461cc4fa7510454391dc170f60135449

Team Coordination

  • This PR introduces a new library: double check it's MIT/Apache2/permissively licensed

Checklist

  • Add tests: none needed
  • Designate a primary reviewer: @BLoe

@johnnymetz johnnymetz self-assigned this Jun 6, 2023
src/icons/list.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #5865 (d79b859) into main (86b8a0a) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main    #5865   +/-   ##
=======================================
  Coverage   66.13%   66.13%           
=======================================
  Files        1083     1083           
  Lines       33403    33404    +1     
  Branches     6298     6298           
=======================================
+ Hits        22090    22092    +2     
+ Misses      11313    11312    -1     
Impacted Files Coverage Δ
src/icons/getSvgIcon.ts 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

@twschiller
Copy link
Contributor

twschiller commented Jun 7, 2023

I'm not 100% happy with my implementation. Writing empty files to the final bundle seems hacky. Instead of writing several empty files, I tried creating a webpack loader that creates a file that contains all icon names but couldn't get it to work, see https://stackoverflow.com/questions/76418333/create-webpack-loader-to-write-icon-filenames-to-a-single-file

@johnnymetz how does https://v4.webpack.js.org/loaders/raw-loader/ work? Presumably that would be a way to return the CDN URL to the list.ts file without writing anything to disk? https://github.com/webpack-contrib/raw-loader/blob/master/src/index.js.

Or is the file emit call somehow necessary? It doesn't seem necessary to emit a .txt file of icons. As webpack could just put the strings into the bundle that list.ts goes into

If we can't get a loader to work, we can always delete the generated files on CI before zipping up. I want to remove any unused small files though, because small files seem to drastically slow down the Rainforest VMs. (Although we can test with your PR branch and see)

@johnnymetz johnnymetz force-pushed the feature/3475-pull-icons-from-cdn branch from 52a5b94 to 12969b8 Compare June 7, 2023 19:32
@johnnymetz
Copy link
Collaborator Author

johnnymetz commented Jun 7, 2023

@twschiller was able to figure out how to do it using the asset/resource module. PR is much cleaner now, doesn't include a custom webpack loader and doesn't include empty SVG icon files (the only folder in the dist/user-icons folder is custom-icons - see image below).

Screen Shot 2023-06-07 at 3 51 09 PM

@johnnymetz johnnymetz changed the title #3475: Pull icons from CDN and remove from bundle Pull icons from CDN and remove from bundle Jun 7, 2023
@twschiller twschiller added this to the 1.7.30 milestone Jun 7, 2023
@github-actions
Copy link

github-actions bot commented Jun 7, 2023

When the PR is merged, the first loom link found on this PR will be posted to #sprint-demo on Slack. Do not edit this comment manually.

@johnnymetz johnnymetz merged commit cf0839a into main Jun 7, 2023
17 checks passed
@johnnymetz johnnymetz deleted the feature/3475-pull-icons-from-cdn branch June 7, 2023 21:04
@fregante fregante mentioned this pull request Mar 14, 2024
2 tasks
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.

None yet

2 participants