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

Introduce code-splitting to web app #81

Closed
macklinu opened this issue Sep 14, 2018 · 9 comments
Closed

Introduce code-splitting to web app #81

macklinu opened this issue Sep 14, 2018 · 9 comments

Comments

@macklinu
Copy link
Contributor

Right now, running npm run build in the server/ project generates one main.min.js bundle weighing in at 1.43 MB.

Version: webpack 3.12.0
Time: 14165ms
       Asset     Size  Chunks                    Chunk Names
 main.min.js  1.43 MB       0  [emitted]  [big]  main
main.min.css   133 kB       0  [emitted]         main

If some code-splitting refactors are introduced, that could reduce the overall bundle size (a potential performance improvement). Some thoughts on how this could be introduced / places to look at code-splitting:

The first step would be adding support for the dynamic import() function to Babel using @babel/plugin-syntax-dynamic-import for supporting dynamic imports being compiled by Webpack + Babel (my understanding is that Webpack uses import() statements as code split points).

I've found react-loadable a helpful library for defining code split points for React components, as you can change the way you import the component but continue to render it the same way and benefit from code splitting.

// ListItem.js

// instead of this
import ReactJson from 'react-json-view'

// lazy load instead
import Loadable from 'react-loadable'
const ReactJson = Loadable({
  loader: () => import('react-json-view'),
  loading: LoadingComponent, // TODO
})

I think a good first component to code split out could either be <ListItem> or the default import from react-json-view, since that component is not visible until a user expands the list item to view payload data. bundlephobia reports that react-json-view is 36.1kB minified and gzipped, so reducing that size from the initial bundle would likely improve load/parse times.

Some other places where code-splitting could be worth investigating to see if there are potential bundle size wins:

  • Split the vendor libraries into their own chunk. This would contain 3rd party libraries and would be cached in the user's browser unless they emptied their cache or smee shipped a new version of the vendor bundle (updated libraries), which could improve loading time.
  • Explore lazy loading icons in the <EventIcon>'s icon map. I'm guessing the icons are pretty small, but maybe loading them only when they are needed or at some point after the initial bundle has been downloaded could be a minor performance improvement?

https://github.com/probot/smee/blob/79cfefe731d23b68abed09ca7d67daa37897f628/server/src/components/EventIcon.js#L24-L46

One open question would be around react-loadable's loading component and what that would look like in the context of where different app components are being loaded dynamically, but I'm probably getting ahead of myself of that one. 😅

Would you be interested in pull requests to help with this enhancement? Any thoughts around this?

@stale
Copy link

stale bot commented Dec 13, 2018

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

@stale stale bot added the wontfix label Dec 13, 2018
@macklinu
Copy link
Contributor Author

Still relevant - looking for input! 😄

@stale stale bot removed the wontfix label Dec 13, 2018
@gr2m
Copy link
Contributor

gr2m commented Dec 14, 2018

I’d say it depends on how much complexity is added to the project. I agree that 1.43 MB is a lot but maybe there are other ways to reduce the size, and maybe it’s not a problem for users of smee.io as it’s mostly developers

@macklinu
Copy link
Contributor Author

maybe it’s not a problem for users of smee.io as it’s mostly developers

That's a good point. This would definitely be a for-fun thing for me to help improve bundle sizes / performance, but I agree that it is likely not mission critical in the same way small bundle sizes would be for other applications.

Happy to PR a before and after by introducing code splitting to get a sense of the tradeoff between complexity and bundle size improvements for others to take a look. 😄

@gr2m
Copy link
Contributor

gr2m commented Dec 14, 2018

If it’s for excercise, then by all means, send pull requests, just please don’t be mad if we don’t accept it because our main objective is to keep the project easy to maintain, it's only a side project.

I say "we" but I don’t want to take any credit, I didn’t build any of this, just helping out myself :)

@macklinu
Copy link
Contributor Author

Most definitely, no offense, just looking to engage in open source projects that I use / enjoy and see what I can help with from both practical and nice-to-have standpoints. ❤️

@tcbyrd
Copy link
Contributor

tcbyrd commented Dec 15, 2018

If it’s for exercise, then by all means, send pull requests,

I echo this. I welcome the improvement, but don't know enough about what kind of maintenance issues this might introduce, and it's not clear if code-splitting would really save us a whole lot here. i wonder if the bigger savings would come from cutting down the overall bundle size (tree-shaking, minification, removing dependencies, etc.) and seeing if it's an acceptable size after that?

@JasonEtco
Copy link
Member

JasonEtco commented Dec 15, 2018

I'm a little sad about that 1.43mb - its way way more than it should be, and while code splitting will help, it's not really fixing the problem. 10000% feel free to try it out for your own sake, but I don't think its the right approach to fix this.

I ran this through webpack-bundle-analyzer, and here's what it looks like:

image

You can see that a lot of the size comes from highlight.js, which is way overkill considering we're using it on static text. react-octicons is also gigantic; we should switch to the officially supported library. There's a lot more to learn from this, but that's my immediate takeaway.

I've opened up #86 to tackle those fixes!

@macklinu
Copy link
Contributor Author

I think a lot of the underlying problems of this have been addressed in #86, and if there are further improvements to lowering bundle size, let's open them up in separate issues. Closing this one. ❤️

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

No branches or pull requests

4 participants