codemod(svgs): Convert imported SVGs to react components

[dac09]

This codemod will find all the cases where SVGs are imported as used as components, and then:

1. Generates a react component with SVGR (see fixtures for example)
2. Replaces the import to the svg file with an import to the new React component

e.g.

- import Bazinga from '../bazinga.svg'
+ import Bazinga from '../BazingaSVG.jsx'

const myComponent = () => {
  // ...
  <Bazinga/>

Don't forget that this is needed no matter if you use vite or webpack

Also:
- fixes codemod template
- updates testUtils to output more verbosely
This got pulled out into a separate PR

Todo:

• test on a real project
• check output with Webpack
• check output with Vite
• deal with SVGs as renderProp
• deal with unused SVG imports
• add svgr as dependency, instead of using npx because tests timeout
• deal with src imports
• component names should be sanitized

[KrisCoulson]

Just to let you know that I am pretty sure this change breaks styling.

if you have an svg like this

<svg stroke="currentColor" fill="currentColor" stroke-width="0" viewBox="0 0 24 24" height="1em" width="1em" xmlns="http://www.w3.org/2000/svg">
	<g>
		<path fill="none" d="M0 0h24v24H0z"></path>
		<path d="M12 22C6.477 22 2 17.523 2 12S6.477 2 12 2s10 4.477 10 10-4.477 10-10 10zm-1-11v6h2v-6h-2zm0-			4v2h2V7h-2z"></path>
	</g>
</svg>

When you add a style like color: red the svg is red.

but if you put it in an img tag I don't think it respects the color anymore.

also see
WICG/proposals#50

[dac09]

Just to let you know that I am pretty sure this change breaks styling.

if you have an svg like this

<svg stroke="currentColor" fill="currentColor" stroke-width="0" viewBox="0 0 24 24" height="1em" width="1em" xmlns="http://www.w3.org/2000/svg">
	<g>
		<path fill="none" d="M0 0h24v24H0z"></path>
		<path d="M12 22C6.477 22 2 17.523 2 12S6.477 2 12 2s10 4.477 10 10-4.477 10-10 10zm-1-11v6h2v-6h-2zm0-			4v2h2V7h-2z"></path>
	</g>
</svg>

When you add a style like color: red the svg is red.

but if you put it in an img tag I don't think it respects the color anymore.

also see WICG/proposals#50

True, and thanks for pointing this out. I knew there were cases where its different, I just couldn't remember what!

It depends on what people use for styling SVGs ofcourse, but if they used styled components or tailwind (or just classnames) it should carry over.

We can merge this in when we decide how to proceed.

[dac09]

@jtoar do you want to give this a once over? We're good to merge.

[jtoar]

Sorry @dac09 I don't have the bandwidth right now, deferring to you and @KrisCoulson

[dac09]

Merging this in, will test with Kris tomorrow - maybe when we write the SVG usage guide.

[Tobbe]

This sounds good, but I don't see these changes in the code. Did you decide not to do those changes after all?

[Tobbe]

I think we should leave SVGs alone that pass non-img props to the generated svgr component. Makes no sense to me to codemod into something we know is broken. Like <MyIcon strokeWidth={2} /> -> <img ... strokeWidht={2} />. We know the generated img wont work, so I think it's better to leave the original code.

[Tobbe]

Also, did we consider using https://github.com/pd4d10/vite-plugin-svgr instead of introducing this breaking change?

[dac09]

Also, did we consider using https://github.com/pd4d10/vite-plugin-svgr instead of introducing this breaking change?

We’d need to use a babel plugin so that prerender still works. It’s a conscious decision not to, as discussed before, we want to be closer to Vite’s defaults. There’s a lot of nuance to this - and can create edge cases where rendering breaks (e.g. when using svg component in .js files - why I do not know). Ofcourse users are free to configure their project however they wish. I think https://github.com/JonasKruckenberg/imagetools/blob/main/docs/guide/getting-started.md is what we should be aiming for in the future, using directives feels a lot better UX and avoids the pitfalls of SVGR.

The plan is to remove support, but provide a guide for how to set up SVG components in their project. This means the edgecases aren’t a redwood problem, but a project specific problem (which is a much better problem to have)

This sounds good, but I don’t see these changes in the code. Did you decide not to do those changes after all?

Pulled those changes into a different PR that got merged.

I think we should leave SVGs alone that pass non-img props to the generated svgr component. Makes no sense to me to codemod into something we know is broken. Like <MyIcon strokeWidth={2} /> -> <img ... strokeWidht={2} />. We know the generated img wont work, so I think it’s better to leave the original code.

There’s 1000s of permutations of how attributes are used, and the safest option is to keep all the existing attributes - _with _the caveat that styling may change. SVGR can process SVGs a little differently - and can be brittle. See #8291. Adding strokeWidth etc. is harmless, even if not ideal.

[Tobbe]

It’s a conscious decision not to, as discussed before, we want to be closer to Vite’s defaults. There’s a lot of nuance to this

When we talked about this at the core team meeting I didn't have the full picture

1. I didn't think about the "pass svg-specific props" use-case. This is a unique feature of SVGs that RW users used to have access to, but won't any more.
2. I didn't know how easy it'd be to maintain the same suport with Vite (just add a plugin)
3. I didn't know that this wasn't default for webpack either but rather something someone at some point thought was a valuable addition to RW

Removing a feature means more time is spent in the team talking about it, and more time is needed for users updating their projects, and more friction is introduced in the upgrade process meaning users are less likely to upgrade. I think it needs to be more clear that over time the initial cost will be worth it. Both for us and for our users.

But I also know that sometimes I'm overly accommodating to our users need, and willing to go too far to make it easy for people at the cost of code complexity etc.

Pulled those changes into a different PR that got merged

Awesome! I like it when PRs are smaller and more focused 🙂 I made a note of it in this PR's description

There’s 1000s of permutations of how attributes are used, and the safest option is to keep all the existing attributes - _with _the caveat that styling may change. SVGR can process SVGs a little differently - and can be brittle. See #8291. Adding strokeWidth etc. is harmless, even if not ideal.

I'm just worried that someone runs the codemod and then a while later realizes that half of their svgs have the wrong size or color. And they won't know which ones until they see them in their app. It's easier to find ones that weren't codemodded because they'll have a project that won't build until they fix them all.

[dac09]

@Tobbe want to review again please?

Worth trying out, because it does a lot of funky things. Looks like tests are timing out because of the use of npx.

The alternative is to add svgr as a dependency to the codemod package - will do that in a bit.

[Tobbe]

I tried this on an example project I set up now, and this is the output I got

What does "7 unmodified" mean?

Using RW src/-style imports doesn't seem to work

[Tobbe]

Just realized it's also having problems with filenames with dashes in them. The generated component name isn't valid.

[dac09]

What does "7 unmodified" mean?

It means JSCodeshift did not modify them.

Thanks, adding Todos:

• deal with src imports
• component names should be sanitized

[Tobbe]

Suggested change

// Here we know exactly which file we need to transform, but often times you won't.

We use globbing because we don't know exactly what files we need to transform, right? So the comment is wrong.

[Tobbe]

It means JSCodeshift did not modify them.

"7 source files matched targetPaths but were not modified". Something like that would make it more clear I think 🙂 Not something we can/should fix. Just jscodeshift that could work on their output messages. Because first I was like "7 unmodified? I don't even have 7 SVGs in total!" 🙂