IconNameSubset duplicate name issue when using multiple icon sets

[andy-polhill]

Hi, I believe we may have uncovered a potential breaking change while making a switch from 5.0.1 to 5.7.1, or alternatively it may be a misuse from our side.

In our project we have two sets of icons, icons and brand-icons. We isolate the two by running different svg-to-ts commands and generating separate files.

this change in v5.6.0 which introduced the IconNameSubset causes us a bit of an issue is we now have two exported types with the same name.

An unhandled exception occurred: projects/canopy/src/public-api.ts:25:1 - error TS2308: Module './lib/icon/index' has already exported a member named 'IconNameSubset'. Consider explicitly re-exporting to resolve the ambiguity.

There are probably a few things we could to fix it in our project, but I thought I would raise it here first to see if you think it had any validity as a bug. If so I'm happy to raise a PR to try and fix it.

One route would be to provide some kind of cli option to prefix the type in the same way it's done for the interface name. With a new option a breaking change could be avoided by deferring back to the default if no option is provided, the downside is that it might be a bit of an obscure option.

[nivekcode]

Hi @thatguynamedandy

Thx a lot for reporting this issue. You are absolutely right. That's an error on our site. We didn't think of the combination of the helper type IconNameSubset in combination with multiple configurations.

So yes - we need to find a solution for this.

1. We could prefix the helper type.
2. We could add a flag to enable or disable it - So you could enable it per configuration.
3. We would generate it only once into a shared folder or something.

The first option is the easiest to realize. But then you would have two helper types with different names that do the same. Disabling them would also be easy to realize. But then you need to be aware of enabling and disabling them. Generating them into a directory also would only be helpful in combination with option two. Thoughts?

I will also try to think again about the 3 options. Maybe there's an even better way.

[nivekcode]

After looking into it I think it makes sense to add a name prefix and also an option to disable it. So basically I would even switch to disable it by default which would be a breaking change.

[nivekcode]

I saw that you forked the repository. Are you already working on this or should I take a look?

[andy-polhill]

I'll endeavour to take a look if thats ok?

I spent a bit of time looking into some options with typescript but didn't achieve too much. I was trying to focus on the ideal outcome and seeing how we might get there. I wondered if the ideal option would be this.

export type IconNameSubset<T extends Readonly<BrandIcon[]|Icon[]>> = T[number]['name'];

Which would allow us to use the same type for both Icons and BrandIcons (which in our case are the two types), meaning you could do this...

export type someIcons = IconNameSubset<[typeof myIconA, typeof myIconB]>; ✅
export type someBrandIcons = IconNameSubset<[typeof brandIconA, typeof brandIconB]>; ✅
export type thumbBrandIcons = IconNameSubset<[typeof myIconA, typeof brandIconA]>; ❌ //no mixing of types

However I don't think there is anyway of doing this as the two icons are run on completely independent commands and I couldn't think of a clever typescript way of achieving that outcome.

So the option that you mentioned may be easiest, would it make sense to use the interfaceName as the prefix?

    export type ${interfaceName}NameSubset<T extends Readonly<${interfaceName}[]>> = T[number]['name'];

This way we wouldn't need to add any more options and it would still always export a uniquely named type helper. It would be a breaking change though.

Depending upon the appetite for breaking changes we could avoid it by adding an interim option prefixTypeHelper: boolean = false, which could be set as true to use the prefix. This would postpone a breaking change if you had more planned, and would only be needed for the minority who are affected by the issue (it would be a little smelly though)

[andy-polhill]

Above is one possible approach, only raised as a draft more than happy to alter it.

[nivekcode]

Hi @thatguynamedandy Thanks a lot for the explanation and the draft. I think this is the best solution. In case somebody needs to have the same Icons he could still do the following:

export type thumbBrandIcons = someIcons | someBrandIcons
I looked at the PR and it looks good to me. Also, I think breaking changes are not an issue as long as they are justified. So to me this already looks good. The release of this library is fully automated. Could you again commit with this commit message format:

fix(scope): message

BREAKING CHANGE: description

This should then automatically release v6.0.0.

[nivekcode]

Thx a lot for your effort 👍

[andy-polhill]

Hi @kreuzerk I moved the PR out of draft, I think the commit message should match the syntax already, I didn't add the optional scope as I noticed it wasn't present in much of the history, but happy to add one.

[nivekcode]

Awesome. Thx a lot 👍 . Great work!

[nivekcode]

@all-contributors please add @thatguynamedandy for code, bugs, doc, ideas

[allcontributors]

@kreuzerk

I've put up a pull request to add @thatguynamedandy! 🎉

[nivekcode]

🎉 This issue has been resolved in version 6.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀