Closed JamesManningR closed 5 months ago
Thank you for opening this issue @JamesManningR, we want to improve how icons are loaded in the page.
Overall we discourage people to use EbaySvg
since it is a huge component and the server takes time to build it and the browser takes time to parses it. But also to add to this, EbaySvg
should never be part of the bundle. This should be a component only rendered by the server.
What we encourage people to do is to use EbaySvg
as a reference and create your own Svg component with only the icons that the application uses. The problem with this approach is that is manual and is hard to get icons up to date on every skin upgrade. This exact same problem would happen with the proposed solution, since the icons.svg
needs to be up to date with skin version that is being used in the application adding also the step to publish this icons in the CDN.
If we see how Marko ebayui-core implements icons, they add the definition in cases it wasn't already added. This works for marko because the definition not necessary will be part of the bundle size, but that is not possible in React model where they need to match the rendering part so it must be part of the bundle if we put on the EbayIcon
rendering part.
One possible solution would be having a script to generate the icons that the application uses (similar to what we use today already to generate EbaySvg and types), and they will render that component only on the server.
// scripts/generate-icons.js
import { generateIcons } from '@ebay/ui-core-react/scripts'
generateIcons('/path/to', [
... // list of icons
])
By running the command node scripts/generate-icons.js
it should output a new component svg.tsx
file
/* This file is autogenerated, do not edit - it will be overwritten anyway */
export const SvgIcons = () => (
<svg>
...
</svg>
)
To make it even simpler we could have a ui-core.config.ts (can be a json also, just to have autocomplete and validation of icon name) file and have a scripts for that so application doesn't need to create a scripts/generate-icons
import type { EbayUICoreConfig } from '@ebay/ui-core-react'
const uiCoreConfig: EbayUICoreConfig = {
icons: [
...
]
}
And then can be generated with the command npx @ebay/ui-core-react generate-icons
The main concept between the two is the same (have a script that generate the icons).
And then the application would be responsible to use that component on the server only:
import { SvgIcons } from './path/to/svg.icons.tsx'
const html = ReactDom.renderToString(
<html>
<head>
...
</head>
<body>
<SvgIcons />
<App />
</body>
</html>
)
@abiyasa @darkwebdev @shpandian @montoya332 any thoughts?
@HenriqueLimas That could be combined with hosting a static file, Instead of outputting a new SvgIcons
with the icon definitions, you could output it as a new icons.svg
with a hash suffix. ie icons-i4h1s.svg
so you don't get any issues updating the asset.
If the generateIcons
script is app wide, it might be better to have all of those icons cached, than output them to every page.
As a more involved improvement, we could also add a search script to crawl your project in the same vein of how tailwind searches for your classes (but probably easier since the names are always going to be in the same attribute on the EbayIcon component)
Otherwise, wouldn't it be just as optimal to export all of the icons as individual components? Or svg files with svgr to bridge the definition to component gap?
It's also probably worth writing in the readme that it was never intended to be a part of the bundle, or even better add it as a JsDoc comment so it surfaces that information in your IDE
I've been working on refactoring the
EbayIcon
andEbaySvg
for our use cases.At the moment, the
EbaySvg
component exports all of the possible icons you might need. When thrown onto a site, this leads to an additional ~2'474 dom elements being rendered to the page, regardless of which icons are being used on that page.We could instead host the SVG file separately and make use of browser caching to reduce the bundle size.
as you can see, when loading the svg file first, it responds with a 200 status, the file is still a little large, but 216kb is less than it adds to the js bundle![200 - initial load](https://github.com/eBay/ebayui-core-react/assets/56916766/fa5ea00d-d7c6-464e-a0d3-722185815970)
where this gets better though, is that the browser will then use the cached image from that point onward![304 - next load](https://github.com/eBay/ebayui-core-react/assets/56916766/784eeeb7-ca04-445a-92b8-7e5c28561726)
So far I've managed to come up with this.
There are some limitations to this solution:
In terms of rollout though, there are a few challanges: If you note this line:
You'll notice that the iconFileUrl has to be prepended to the use:href attribute. This isn't how
EbayIcon
currently works, since the svg definitions are rendered directly on the page.A very questionable bandaid fix, is that we can use the xlink:href (currently deprecated but still used and working) as a fallback, for example:
While really questionable, it does make it so a user is able to opt-in for the improvement, and EbaySvg can still be used as normal.
I'd be interested in seeing what you think