Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: build the SVG icons at compile-time #28

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Feb 1, 2023

Fixes #25

This PR changes the build script to generate components that the SVG is inlined within it. This way the components are optimized and built with Solid at compile time removing the overhead of using innerHTML on the client side. I have seen significant performance improvements after using this.

The individually built components are prebuilt with Esbuild for the Web and server to speed up compilation for the users. Workerpool is used to parallelize the post-build and get-icon stages. I have also changed the usage of sync file system calls for the maximum build speed.

Since the bundled files are available, the changes in this PR are backwards compatible with previous versions. The individual icons can be imported as well as shown in the readme.

image

@iosiftalmacel
Copy link

When will this get merged?

@aminya
Copy link
Contributor Author

aminya commented Mar 2, 2023

I have been using this in production without issues. We should wait for @x64Bits to merge it.

@x64Bits
Copy link
Owner

x64Bits commented Mar 11, 2023

Hi @aminya I have been looking at the progress with this PR and I would like to advance and support the build at compile-time of the icons but keeping the current experience when using the library, it would be possible for the API to be maintained with these changes, more specifically talking of destructuring when importing them , I implemented as you suggest on the solid-icons website due to problems when importing them separately and dynamically but I don't know if it is as comfortable in the library.

@x64Bits
Copy link
Owner

x64Bits commented Mar 11, 2023

Another thing that i concern about is regarding the scope of the PR, i think that we can divide the changes on some ones for be able to handle propertly, what do you think?

@aminya
Copy link
Contributor Author

aminya commented Mar 13, 2023

keeping the current experience when using the library, it would be possible for the API to be maintained with these changes, more specifically talking of destructuring when importing them

The current pull request doesn't change the API you can still import the icons you need like before.

import {FiCircle} from "solid-icons/fi"

This PR, makes the granular import also possible in addition to the previous method

import {FiCircle} from "solid-icons/fi/FiCircle"

The other big improvement is the compile-time generation of the components, which removes all the overheads.

@jcramb
Copy link

jcramb commented Apr 4, 2024

@x64Bits is there further help needed or anything else still blocking this PR from being merged?

@aminya
Copy link
Contributor Author

aminya commented Apr 4, 2024

I think the upstream was not interested in the changes. I can publish this under a new name as it is a significant rewrite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting innerHTML is slow
4 participants