[RFC] Adding SVG support

[paulm17]

Currently there are multiple ways of adding SVG support to Blitz.

1. SVGR
2. Babel-plugin-react-svg
3. Next-images
4. Plain image tag

I aim to build a recipe to make it much easier for blitz to support SVG, but also add the correct type definition.

declare module "*.svg" {
  const content: string
  export default content
}

Finally, with a recipe in play, documentation can then be made on how to install and use for new users of Blitz. Prior to finding option 2 it was confusing on what to use.

I am interested in feedback from the community with respect to the other options. Perhaps SVGR or next-images provides better support and facilitates more edge cases?

Please let me know your experience and preference for the recipe to use.

Many thanks!

[flybayer]

Thanks @paulm17!

Is there any reason we shouldn't add the best solution into blitz core so it works out of the box for everyone with zero config?

[paulm17]

Sure, let's do that. I'm not that familiar with the core but happy to figure it out and make a PR.

[paulm17]

So I did a bit of a google and found:

Babel-plugin-react-svg: It's a couple of years since the last update.

SVGR: Has a nice intro to it. https://www.youtube.com/watch?v=geKCzi7ZPkA and last update was 15th Nov.

Really simple to install:

npm install @svgr/webpack --save-dev | yarn add @svgr/webpack --dev

Add to webpack:

{
  test: /\.svg$/,
  use: ['@svgr/webpack'],
}

I was able to switch from babel-plugin-react-svg to SVGR to no noticeable detriments. I think Blitz should use this by default. But I'll give it a while for more feedback.

[MrLeebo]

Probably also would want to add the Jest mock to the new app template so that people won't stumble over that when they try to write tests for components containing SVG imports.

[tmcw]

I've used SVGR on a lot of projects now and it's very solid, including good documentation. Definitely +1 for going with that, if SVG in core makes sense. I think it does make sense, though not sure if this makes more sense in Blitz or Next.

[flybayer]

We already have a jest mock: https://github.com/blitz-js/blitz/blob/canary/packages/blitz/jest-preset.js#L27

[paulm17]

Got a slight issue with SVGR. Not sure if it's one to be honest...

After installing SVGR and I already have in my global types

declare module "*.svg" {
  const content: string
  export default content
}

Works fine, except when I try to add height, width props to the SVG element. I get a typescript error it's not defined in the react component.

No problem I thought. If I change:

import Degrees from "./icons/degrees.svg"

to:

import {ReactComponent as Degrees} from "./icons/degrees.svg"

and change the types:

declare module "*.svg" {
  import React = require("react")
  export const ReactComponent: 
  React.FC<React.SVGProps<SVGSVGElement>>
  const src: string
  export default src
}

I don't get any issues on props. But then I get this:

./app/core/components/colorPicker/sketch/body/gradient/gradientLinearDropDown.tsx
Attempted import error: 'ReactComponent' is not exported from './icons/degrees.svg' (imported as 'Degrees').

I've hit a dead-end on this issue. There is even a next plugin, but I couldn't get anywhere with that.

If someone can take a look at this. I'd be grateful! Thanks

[flybayer]

We can definitely fix the types. Try this:

declare module '*.svg' {
  import * as React from 'react';

  const ReactComponent: React.FunctionComponent<React.SVGProps<SVGSVGElement>>;

  export default ReactComponent;
}

[paulm17]

Using this type instead of the one I supply, gives the error:

Module '"*.svg"' has no exported member 'ReactComponent'. Did you mean to use 'import ReactComponent from "*.svg"' instead?ts(2614)

My issue isn't with the types. It's more about the import error.

[flybayer]

Oh yeah, you said "typescript error" but this is actually a runtime error

[flybayer]

Everyone seems very happy with the proposal to add it to blitz core, so let's go with that.

You'll add the webpack config here: https://github.com/blitz-js/blitz/blob/canary/packages/core/src/with-blitz.ts#L18 and add the dependency to packages/core/ and then we also need to update the new app template global.d.ts

[paulm17]

Sure. Give me a bit though. Real life™ is currently getting in the way. I'll get around to doing it soon!

[undefy-ishaan]

link doesnt work @paulm17

[paulm17]

@undefy-ishaan I don't think this applies anymore with the toolkit

[paulm17]

Zarazas posted this in the RFC chan.

Is it of any concern? Any input?

https://twitter.com/_developit/status/1382838799420514317

[tmcw]

There are some drawbacks of the other approaches: if you use an img tag, external styles won't be able to affect the SVG contents - no more using a text color in order to change the color of an SVG icon that uses currentColor. And the use/defs technique is fairly rare because it's super nitpicky: you have to choose & match ID attributes between the symbol and the use element.

Jason's example also assumes that someone is importing, and not using, a lot of SVG elements. If you have a fancy enough bundle splitting setup, you should be able to avoid that and dynamically import your SVGs.

I think Jason's points are totally valid: the SVGR approach, especially with lots of icons or complex icons, is not as performant as use/defs. But the img approach is a non-starter imho, and inlining/bundling SVGs is extremely common, including on GitHub.

[flybayer]

Thanks @tmcw, makes sense. So maybe we should have a way for people to opt out of svgr for advanced cases.

[hasparus]

I'm not sure if posting in RFC from April isn't a faux pas, but I'd like to voice my concerns about possible bugs.

SVG import can be a footgun unless configured correctly. An SVG exported from design software can contain a suprising amount of id attributes. By inserting it multiple times we invalidate the assumption that ids are unique and get a bunch of rendering issues.

It can be worked around, but I'm not sure if the gain outweighs the pain.

An example of SVG icon I worked with a few years back (redacted for brevity).

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="130" height="130" viewBox="0 0 130 130">
    <defs>
        <path id="a" d="M4.537 37.139L41.878 2.892a9.8 9.8 0 0 1 13.405.148l35.605 34.127A14 14 0 0 1 95.2 47.274V95.9c0 5.799-4.701 10.5-10.5 10.5H10.5C4.701 106.4 0 101.699 0 95.9V47.457a14 14 0 0 1 4.537-10.318z"/>
        <filter id="b" width="111.6%" height="110.4%" x="-5.8%" y="-3.3%" filterUnits="objectBoundingBox">
            <feOffset dy="2" in="SourceAlpha" result="shadowOffsetOuter1"/>
            <feGaussianBlur in="shadowOffsetOuter1" result="shadowBlurOuter1" stdDeviation="1.5"/>
            <feColorMatrix in="shadowBlurOuter1" values="0 0 0 0 0.226735433 0 0 0 0 0.437054197 0 0 0 0 0.687818878 0 0 0 0.126584353 0"/>
        </filter>
        <!-- redacted -->
    </defs>
    <g fill="none" fill-rule="evenodd">
        <g transform="translate(16 8)">
            <mask id="c" fill="#fff">
                <use xlink:href="#a"/>
            </mask>
            <use fill="#000" filter="url(#b)" xlink:href="#a"/>
            <use fill="#E3ECF3" xlink:href="#a"/>
            <!-- redacted -->
        </g>
        <!-- redacted -->
    </g>
</svg>

Related issues:

• the problem — ID collisions with multiple SVG gregberge/svgr#150 (comment)
• more about the problem — Incorrect rendering for multiple SVGs boopathi/react-svg-loader#294
• a workaround — Option to generate unique IDs? svg/svgo#674 (comment)

[flybayer]

Thanks for sharing this!