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.

Sign up for GitHub

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

Jump to bottom

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

Open
mokipedia opened this issue Dec 11, 2023 · 13 comments
Open

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

mokipedia opened this issue Dec 11, 2023 · 13 comments

Comments

@mokipedia
Copy link
Contributor

mokipedia commented Dec 11, 2023 •
edited

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_restore_solid" | ... 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/window_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 StackBlitz
Copy link

stackblitz bot commented Dec 11, 2023

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

@NetanelBasal
Copy link
Member

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
Copy link
Contributor Author

I am investigating rn, as far as my tests go, the change from snake_case to kebab-case is happening here in the AST:

svg-icon/svg-generator/ast.ts

Line 32 in bcceb6e

factory.createStringLiteral(kebabCase(iconName), true),

@NetanelBasal
Copy link
Member

NetanelBasal commented Dec 12, 2023 •
edited

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

@mokipedia
Copy link
Contributor Author

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
Copy link
Member

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

@mokipedia
Copy link
Contributor Author

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

@NetanelBasal
Copy link
Member

I mean in the generated code

@mokipedia
Copy link
Contributor Author

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

@mokipedia
Copy link
Contributor Author

changing the name here would be an option (still breaking change):

svg-icon/svg-generator/tree.ts

Line 71 in bcceb6e

name: iconName,

@NetanelBasal
Copy link
Member

Can you list the possible options, please?

@mokipedia
Copy link
Contributor Author

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

  1. removing kebab-case transformation from AST (breaking)

    svg-icon/svg-generator/ast.ts

    Line 32 in bcceb6e

    factory.createStringLiteral(kebabCase(iconName), true),

    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)

    svg-icon/svg-generator/create-types.ts

    Line 26 in bcceb6e

    return factory.createLiteralTypeNode(factory.createStringLiteral(name, true));

    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)

    svg-icon/svg-generator/tree.ts

    Line 71 in bcceb6e

    name: iconName,

    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
Copy link
Member

I think we can go with option 2 for now

@mokipedia mokipedia mentioned this issue Dec 12, 2023
Merged
3 tasks
NetanelBasal added a commit that referenced this issue Dec 13, 2023
@NetanelBasal
Merge pull request #144 from mokipedia/master
d4b688e 
fix(svg-generator): add kebab-case transform to type generator (#143)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@NetanelBasal @mokipedia