ngneat / svg-icon

👻 A lightweight library that makes it easier to use SVG icons in your Angular Application
https://netbasal.com
MIT License
257 stars 35 forks source link

key type and name missmatch if icon has _ in filename #143

Open mokipedia opened 9 months ago

mokipedia commented 9 months ago

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request
[ ] Other... Please describe:

Current behavior

I am having several icons with _ in the name, e.G.: "window_restore_solid.svg" SVGGenerator creates a similar file "window_restore_solid.ts", which is fine. However, the name field is then "window-restore-solid" instead of "window_restore_solid". The key has a type of "window_restoresolid" | ... so every file with an does not work or shows a type missmatch in IDEs like Webstorm. The problem occurs when the svg generator creates the types.d.ts file inside the node_modules @ngneat/svg-icons/lib folder. cat node_modules/@ngneat/svg-icon/lib/types.d.ts export declare type SvgIcons = 'window_restore_solid';

Expected behavior

key type and name property should be the same

Minimal reproduction of the problem with instructions

https://stackblitz.com/edit/stackblitz-starters-tmwzwm?file=icons%2Fwindow_restore_solid.ts (for whatever reason you cannot have an angular project and a terminal to run the svg-generator so typechecking in stackblitz angular template is limited. Webstorm however will use this type)

you can type cat node_modules/@ngneat/svg-icon/lib/types.d.ts to get the type generated after you run npm run svg

Environment


Angular version: 17.x

Browser:
- [ ] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX

For Tooling issues:
- Node version: 18
- Platform:  Mac, Windows, Linux
stackblitz[bot] commented 9 months ago

Fix this issue in StackBlitz Codeflow Start a new pull request in StackBlitz Codeflow.

NetanelBasal commented 9 months ago

Do you want to open a PR? It's may related to:

https://github.com/ngneat/svg-icon/blob/master/svg-generator/tree.ts#L86

mokipedia commented 9 months ago

I am investigating rn, as far as my tests go, the change from snake_case to kebab-case is happening here in the AST: https://github.com/ngneat/svg-icon/blob/bcceb6eb9fc066f038dc4b00633c5841995e66ba/svg-generator/ast.ts#L32

NetanelBasal commented 9 months ago

That's correct. This code needs to be changed.

mokipedia commented 9 months ago

Now I am unsure how to proceed: 1) remove the kebab-case function from AST 2) add a similar transformation to the create-types.ts so they match again?

1 would be a breaking change, 2 is backwards compatible, but keeps the not really necessary change in key.

NetanelBasal commented 9 months ago

What about changing window_restore_solid.ts to window-restore-solid.ts?

mokipedia commented 9 months ago

this is just a testfile here, our icons come from an npm package that I have no control over

NetanelBasal commented 9 months ago

I mean in the generated code

mokipedia commented 9 months ago

isn't that also a breaking change, since all imports of _ based icon filenames will then be broken after regeneration?

mokipedia commented 9 months ago

changing the name here would be an option (still breaking change): https://github.com/ngneat/svg-icon/blob/bcceb6eb9fc066f038dc4b00633c5841995e66ba/svg-generator/tree.ts#L71

NetanelBasal commented 9 months ago

Can you list the possible options, please?

mokipedia commented 9 months ago

so, as far as I can see, we have 3 options:

1) removing kebab-case transformation from AST (breaking) https://github.com/ngneat/svg-icon/blob/bcceb6eb9fc066f038dc4b00633c5841995e66ba/svg-generator/ast.ts#L32 this will change the keys, so users will need to migrate to new keys. this is probably the semantically correct fix.

2) add kebab-case transformation to types (non-breaking) https://github.com/ngneat/svg-icon/blob/bcceb6eb9fc066f038dc4b00633c5841995e66ba/svg-generator/create-types.ts#L26 this will change only the types and would fix the type missmatch. it is not breaking, but it only mitigates the the types missmatch, not the missmatch in key an filename (which might be fine, if documented). not sure what happens if both files exist (e.g. window_restore_solid and window-restore-solid)

3) add kebab-case transformation to filename (breaking) https://github.com/ngneat/svg-icon/blob/bcceb6eb9fc066f038dc4b00633c5841995e66ba/svg-generator/tree.ts#L71 this will change the keys and filenames, so users will need to migrate to new keys and import statements. not sure what happens if bot files exist (e.g. window_restore_solid and window-restore-solid)

NetanelBasal commented 9 months ago

I think we can go with option 2 for now