Use svgr to generate icons for iconoir-react

[paescuj]

Thanks @lucaburgio for providing this beautiful icon library and thanks @PianoManDanDan for creating the iconoir-react package!

I started using iconoir-react in a React project and noticed that the generated JSX syntax in the React components is invalid (e.g. stroke-width should be strokeWidth) and the svg element is defined twice:
https://github.com/lucaburgio/iconoir/blob/18671eed7e425ab043233e596f90581eae193557/packages/iconoir-react/src/icons/1st-medal.jsx#L7-L22

While looking for a lasting solution to this I stumbled upon svgr (a tool to transform SVGs into React components) which seemed like a perfect fit for iconoir-react.
After playing around and testing svgr, I came up with this pull request which contains the following changes:

• Use svgr to generate the icon React components.
  • Due to the incompatible icons names there is still a bin/build.js script which now copies all icons, fixes the names and runs svgr afterwards. Unfortunately, I had to use the svgr CLI here because the API doesn't provide the possibility to process a whole directory and automatically creating a export file. We could get rid of this script, e.g. by renaming/fixing the icons with incompatible names in the root directory...
  • svgr now creates TypeScript React components instead of using prop-types.
• Switch from rollup to tsup which is much simpler and automatically creates and bundles TypeScript types definition.
  • Note: While the automatically generated types definition aren't ideal, it does work for now. I think it should be changed to something similar as the current definition - this would mean we have to create a custom index.js template for svgr.

The usage is still the same, e.g.:

import { Iconoir } from 'iconoir-react';

const App = () => {
  return <Iconoir />
};

However with this implementation you are now able to pass all valid SVG props to the icon component (e.g. width, height, color, id, ...), for example:

// Overriding color which is 'currentColor' by default
<Iconoir color="red" aria-label="Iconoir Icon" />

The only thing which is really different is that instead of the option size you now have to use width/ height to change the icon size and the default is now "1.5em" (equals to 24px when font size is 16px) instead of "24px". I prefer this solution because it makes the icons kind of responsive, especially in text sections:

// Icon adjusts to font size
<div style={{ fontSize: '20px' }}}>
  Some text
  <Iconoir  />
</div>

// Overriding the size, e.g. to static value of 24px as before
<Iconoir width="24px" height="24px" />

Tested with both, a JavaScript and TypeScript React project.

Please let me know what you think! If you're happy with those changes I can update the README as well.

Todo:

• Update README
• Create issue to track enhancement of automatically generated types

[paescuj]

If you would like to test it, I've published it to the GitHub registry:
npm install @paescuj/iconoir-react --registry https://npm.pkg.github.com --legacy-peer-deps

[PianoManDanDan]

I'll take a proper look when I have a moment later on, but from the quick skim over I've just done it looks good and looks like it fixes some of the issues and limitations I had made a mental note of but not yet got around to fixing properly (including the stroke width problem we were talking about earlier @lucaburgio).

If we did want to keep the size prop, we could maybe extend the SVG props type to have a size and shove it into the height and width properties if height and width are undefined. This could potentially be done along with setting up the custom index.js template for svgr if we decided to go down that route

[paescuj]

Thanks @PianoManDanDan! Looking forward to your feedback 😃

You're right! If you want to keep the size prop we could use the custom template option from svgr. However, I generally prefer to keep things as simple as possible 😉

[lucaburgio]

Thank you @paescuj, that's super interesting! And thank you @PianoManDanDan for taking care of this! I'll follow the thread 🙏🏻

[PianoManDanDan]

@paescuj I've left a couple of comments. For the most part it looks good, only one thing that doesn't quite look right. I'm also having a few issues running some of the commands at the moment, but suspect it's due to me using node 12, so if you could let me know what version of node and npm you're using I'll update and try again. I agree with what you say about the automatically generated types and would like to change it in the future, but it is something we can come back to at a later date.

@lucaburgio it looks like this also doesn't fix the stroke width problem. The react library will try to add the stroke width onto the root svg element, but because you're adding it to each of the path elements at the moment, the <path> stroke widths overwrite the root <svg> stroke width. Is it possible to add the stroke width onto the root elements at all, or are you sometimes using different stroke widths in the same icon?

[lucaburgio]

@PianoManDanDan the icons are using the same stroke-width everywhere, so yes, I would remove the stroke-width property from all the paths and move it in the root element. This would also make the icons clearer.

Let me know if you want me to fix them now directly in the master.

On a side note: Should we do the same for the stroke="currentColor" in order to let you change the color too?

[paescuj]

@PianoManDanDan Thank you very much for your feedback!
I'm using Node.js v16 because I rely on some of the new features... Sorry for that! I've tested the build script with Node.js v14 and it's working except that npx seems to log to stderr.
I recommend to use at least v14 (current LTS). With v14 prepublishOnly in package.json as well as fs.rm should both be supported.
Should we us v14 for this project (Node.js v16 is going to be LTS in October 2021)? In that case I can regenerate the package-lock.json file which is currently in a format targeting v16.

Thanks for pointing out the error - it's fixed now. Besides that I did some other improvements...

I agree with what you say about the automatically generated types and would like to change it in the future, but it is something we can come back to at a later date.

Great! I think the best thing would be to track this in an issue as soon as we've merged this pull request.

@lucaburgio

On a side note: Should we do the same for the stroke="currentColor" in order to let you change the color too?

This is not absolutely necessary, because currentColor inherits the value from the root/parent elements and we are already able to set the color property on the root element.

[lucaburgio]

@PianoManDanDan @paescuj Just merged 13af1ea. Now all the icons have stroke-width property applied only to the root. ✅

[paescuj]

Thanks @lucaburgio! I've rebased this pull request and regenerated the icons - it seems to work perfectly with your stroke-width change 😄👍

[paescuj]

FYI: Republished latest version to GitHub registry:
npm install @paescuj/iconoir-react --registry https://npm.pkg.github.com --legacy-peer-deps

[lucaburgio]

What do you think @PianoManDanDan ? Can we proceed?

[PianoManDanDan]

Sorry to be a pain but is it possible for you to regenerate the package-lock.json using node 14/npm v6.14.13 @paescuj? By the looks of things with node 16 they've updated how package-lock files are generated and it gives a warning and makes changes to the package-lock if I run an npm install at the moment (using node 14). I think it's a good idea to use node 14 for now as that's the current LTS (we can look at moving to node 16 in October if we want to), so think we should use 14 to generate the package-lock's.

Also for some reason the npx command logs stuff to stderr when running npm run build, but logs to error when running it as part of the prepublish command. If we keep the prepublish command in at the moment it will completely prevent publishing to npm as the error stops the publish from happening. Does npx still log to stderr/error in node 16? I suggest we remove the prepublishOnly script for now and either find a workaround or wait until node 16 becomes LTS (assuming node 16 has fixed this issue - I'm assuming it has as you're publishing it successfully to your scoped package repository).

I agree with what you say about the automatically generated types and would like to change it in the future, but it is something we can come back to at a later date.

Great! I think the best thing would be to track this in an issue as soon as we've merged this pull request.

I agree. Once this is merged in we can raise it as an issue to track.

[paescuj]

Sorry to be a pain but is it possible for you to regenerate the package-lock.json using node 14/npm v6.14.13 @paescuj? By the looks of things with node 16 they've updated how package-lock files are generated and it gives a warning and makes changes to the package-lock if I run an npm install at the moment (using node 14). I think it's a good idea to use node 14 for now as that's the current LTS (we can look at moving to node 16 in October if we want to), so think we should use 14 to generate the package-lock's.

Sure 👍 I've regenerated the package-lock.json file with node 14.

Also for some reason the npx command logs stuff to stderr when running npm run build, but logs to error when running it as part of the prepublish command. If we keep the prepublish command in at the moment it will completely prevent publishing to npm as the error stops the publish from happening. Does npx still log to stderr/error in node 16? I suggest we remove the prepublishOnly script for now and either find a workaround or wait until node 16 becomes LTS (assuming node 16 has fixed this issue - I'm assuming it has as you're publishing it successfully to your scoped package repository).

Yes, pretty strange... I've removed the prepublishOnly step for now and we can add it back later on. No, npx doesn't log to stderr anymore in node 16.

[PianoManDanDan]

All looks good to me. Don't worry about updating the package README @paescuj. When this gets merged in, I'll publish v2.0.0 to npm and update the README then.

All good to go @lucaburgio 👍

[paescuj]

Okay, thanks @PianoManDanDan!

[lucaburgio]

That's great! thank you guys 🙏🏻