[Bug]: SVG conversion back and forth.

[ampersarnie]

What happened?

@g-elwell had mentioned on #33 during testing of that PR about conversion of SVGs going from to a string and then back to a component. We should investigate this and look into impacts and changes that are required. Gareth has gave reference to this as config we're likely to require.

Which environments are you experiencing the issue on?

CI (CircleCI, Travis, etc), CLI (macOS, Windows, etc)

Relevant log output

No response

[g-elwell]

I've looked into this further and our current method of handling SVGs isn't necessarily an issue, but there are some changes/improvements we could make.

For context, we may want to use SVGs in two different situations:

• As a URL on an img src
• Inline as a React component

Our existing config allows for both methods. Our two ways of importing SVGs are:

import mySvg from './images/my-svg.svg'; // import as a URL
import { ReactComponent as MySvg } from './images/my-svg.svg'; // import as React component

During import mySvg from './images/my-svg.svg':

1. SVG is ran through @svgr/webpack
2. Output from this is ran through url-loader
3. The default export from my-svg.svg becomes a URL string
4. We import this URL string

During import { ReactComponent as MySvg } from './images/my-svg.svg':

1. SVG is ran through @svgr/webpack
2. Output is ran through url-loader
3. A named export from my-svg.svg called ReactComponent has been injected by @svgr/webpack
4. We import this component and rename it at the same time

So we aren't actually converting SVGs back and forth, but it does look as though @svgr/webpack could be over-processing files.

The SVGR documentation recommends using a Webpack config that will process SVGs differently depending on whether or not a resourceQuery (e.g. ?url) is present. This results in two separate processing paths and should be more performant.

However this would introduce a breaking change as our existing imports would need to be updated to the new syntax, such as:

import mySvg from './images/my-svg.svg?url'; // import as a URL
import MySvg from './images/my-svg.svg'; // import as React component

Create React App actually uses the same approach as our build-tools, so without measuring the performance impact I'd be pretty confident it's not going to be a huge issue. I would therefore argue that it's not worth introducing a breaking change here, however according to the SVGR docs:

url-loader and file-loader are deprecated over Asset Modules in webpack v5. It is widely encouraged to use resourceQuery method described before.

If this is the case and those loaders aren't going to be around forever, it would possibly make sense to introduce this change now rather than later when more projects have adopted the build-tools.

I've created a branch (fix/svg-handling) to test the new approach out on a project and it works as expected, but I'm interested in whether you think it's worth making the change or not?

[ampersarnie]

Thanks for the expansive report on this @g-elwell. I definitely think it's worth making the change. It seems to me that we need to get this change in sooner rather than later as this is a breaking change that we already know about and will need to make, so best do it now. Can you open up a PR with your branch so I can review and assess from there? I'd also take a look at the Integration/Example Site that I added as part of #41 and add the example there also, just so we can keep up with integration testing.

[g-elwell]

@ampersarnie I've created #45 and checked out the example site. The SVG import in the example site uses the new syntax we're proposing so doesn't need to be updated. Is the example site just for reference or is there a way to build and run tests on it?

[ampersarnie]

@g-elwell The example site is there both for reference and to ensure we've got some basic integration testing. It runs in CI and does a build with the main commands, so if the build fails we know. It'll be getting expanded into something fuller as soon as possible really.

[g-elwell]

@ampersarnie Got it thanks and happy to help with any of that if needed. The SVG import in the example site will produce a runtime error but it will be fixed if/when #45 gets merged. Otherwise this line would need to be changed to:

import { ReactComponent as LogoSVG } from '../static/logo.svg';