x64Bits / solid-icons

The simplest way to use icons in SolidJS
https://solid-icons.vercel.app
MIT License
284 stars 18 forks source link

Attribute view box is missing since solid-js 1.6 #15

Closed pmwmedia closed 1 year ago

pmwmedia commented 1 year ago

Bug:

When using solid-js 1.6.0, the attribute viewBox is missing for all SVGs.

Workaround:

Downgrading solid-js to version 1.5.9 or earlier.

pmwmedia commented 1 year ago

Is this the cause for the bug: https://github.com/solidjs/solid/issues/1296?

oscartbeaumont commented 1 year ago

I also had this issue but even after downgrading SolidJS to 1.5.9 many icons were still displaying weirdly. I ended up resetting back to an old lock file of mine in had in Git. The issue @pmwmedia linked said it should be fixed if you remove you lock file and reinstall but that didn't seem to be the case on my machine.

outbackStack commented 1 year ago

downgrading to 1.5.4 works for me...

x64Bits commented 1 year ago

Hi everyone, if there seems to be a problem when destructuring the props, do you think it is convenient to establish a specific version of solid-js as a dependency for solid-icons?

Because right now { "solid-js" : "*" } is set for convenience and directly use the same user version but if this type of problem can happen I think it's better to have a fixed version.

outbackStack commented 1 year ago

hi @x64Bits,

Thanks for creating this for us :-)

Feather icons has specific version but since solid changes every 3 months it's best to leave it as it is. We can just report the problem if people run into.

Otherwise, you will need to update this constantly....

Schlumie commented 1 year ago

Will this be fixed? Downgrade cant be the solution?

icode commented 1 year ago

downgrading to 1.5.7 works for me...

x64Bits commented 1 year ago

I'm sending version 1.0.3 which adds solid-js <=1.5.7 as a dependency to avoid the problem, I did not want to have a direct dependency on solid since it was implicit but if is not added the library it breaks, If anyone has a better idea I'll leave this thread open for suggestions.

outbackStack commented 1 year ago

@x64Bits with v1.0.3 you are forcing everyone to wait for this package to get updated before they can update solid-js. Breaking changes like this rarely occur for icons so I would suggest to leave it as it is and we just report the bug.

x64Bits commented 1 year ago

@outbackStack I understand, because I don't like the idea of forcing a version either, but it tells me that people will choose to downgrade solid-js in order to use this library? Doesn't it sound more logical to update this lib to make it work?

outbackStack commented 1 year ago

@x64Bits I've seen packages where people don't maintain it anymore and we are all stuck even though the latest version still works. It's up to you on how much time you have to maintain this. I'm just thinking it will be easier for you without dependency requirement....

Anyhow, if it does not work with the latest version of solid-js, issues will be created by users of this package.

Thanks again for creating this.

icode commented 1 year ago

@outbackStack I understand, because I don't like the idea of forcing a version either, but it tells me that people will choose to downgrade solid-js in order to use this library? Doesn't it sound more logical to update this lib to make it work?

We are waiting for solidjs to fix this problem!

outbackStack commented 1 year ago

@icode @x64Bits I'm sorry I didn't realised the issue was with solid-js new destructing props that can only be fixed by them....

pmwmedia commented 1 year ago

According to @ryansolid in https://github.com/solidjs/solid/issues/1296#issuecomment-1296143145. All spread issues should be fixed in 1.6.1. Unfortunately, the view box is still missing for all SVGs when using solid-icons 1.0.3 together with solid-js 1.6.1.

Ok 1.6.1 fixed all the open issues we have reported with reproductions against spreads. Since the original issue wasn't actually an issue I'm going to close this one. And if you find issues against the new version please report them.

icode commented 1 year ago

According to @ryansolid in solidjs/solid#1296 (comment). All spread issues should be fixed in 1.6.1. Unfortunately, the view box is still missing for all SVGs when using solid-icons 1.0.3 together with solid-js 1.6.1.

Ok 1.6.1 fixed all the open issues we have reported with reproductions against spreads. Since the original issue wasn't actually an issue I'm going to close this one. And if you find issues against the new version please report them.

Yes, I use solid-js 1.6.1, it not fixed this problem.

x64Bits commented 1 year ago

Everything seems to indicate that the changes I made in 1.0.3 had no effect in general, mergeProps seems to help fix this, i'm submitting another version for this in the PR#17 feat: add unit tests.

pmwmedia commented 1 year ago

I can confirm that version 1.0.4 fixes this issue.

@x64Bits Thank you very much for the bug fix!