Feature request: Make SVG with JS packages resilient to multiple instances of fontawesome-common-types

[devoto13]

Is your feature request related to a problem? Please describe.

The problem was reported in FortAwesome/angular-fontawesome#125. Basically when users update icon package, but not fontawesome-svg-core package, they end up with the cryptic error message:

Argument of type 'IconPack' is not assignable to parameter of type 'IconDefinitionOrPack'

And no simple way to solve it without dirty hacking with package manager and lock files.

This repository provides and example of how this happens (steps 1 and 2).

Describe the solution you'd like

Make IconDefinition and IconPack independent from the core icons, so even if there are multiple instances of fontawesome-common-types package, the types are still assignable to each other because of duck typing.

This change should not cause any loose of type safety as types are loosened on the definition side (which users normally don't use). The only place where completion is lost is when passing IconLookup to the icon() function, because IconLookup type is completely covered by IconDefinition and therefore completely erased. I've discovered this during late testing and this is serious problem. But I'll post this issue anyways just to start a discussion around the problem. Maybe we should just accept this tradeoff... Or deprecate IconLookup in favour of [IconPrefix, IconName] syntax, which will still have completion.

This change also makes it easier for users to define custom icons as there is no need to do any casts on definition side. E.g.

export const faDummy: IconDefinition = {
  prefix: 'fac' as IconPrefix,
  iconName: 'dummy' as IconName,
  icon: [512, 512, [], 'f030', ['M50 50 H412 V250 H50 Z', 'M50 262 H412 V462 H50 Z']],
};

becomes

export const faDummy: IconDefinition = {
  prefix: 'fac',
  iconName: 'dummy',
  icon: [512, 512, [], 'f030', ['M50 50 H412 V250 H50 Z', 'M50 262 H412 V462 H50 Z']],
};

The only breaking change is that IconDefinition is no longer assignable to IconLookup, which continues to use type unions for core icon and package names to provide completion on the usage site. This also makes updated icons/packages incompatible with not updated versions of fontawesome-svg-core, but this is more or less how it is today with FortAwesome/angular-fontawesome#125. Just that it will affect more people during the migration phase.

See step 3 in the repository for the reference implementation of this feature.

Describe alternatives you've considered

• Making fontawesome-common-types package a peer dependency of fontawesome-svg-core and all *-svg-icons packages to ensure that there is only one instance of fontawesome-common-types is installed. While this solves the problem, it requires extra effort and cognitive effort to maintain correct version and makes installation of FA even more complex.
• Constructing IconName and IconPack types by relying on TypeScript's declaration merging as shown in https://github.com/devoto13/fa-icons-merge. This is ultimately more complex and more disruptive solution, but has an added benefit that custom icons are supported and type check. Another issue @robmadole pointed in the private chat is that some people rely on completion to discover icons and with this approach only imported icons will be available in completion list.

Additional context
Add any other context or screenshots about the feature request here.

Feature request checklist

• This is a single feature (i.e. not a re-write of all of Font Awesome)
• The title starts with "Feature request: " and is followed by a clear feature name (Ex: Feature request: moar cowbell)
• I have searched for existing issues and to the best of my knowledge this is not a duplicate

[mlwilkerson]

@devoto13 great write-up! Now, let's see if I can catch up to you on understanding the problem and proposed solution 😄

After looking at the demo repo you provided, it looks like one of these type-incompatibility scenarios could happen as easily as when a new FA release has more icons added (combined with the condition that two packages that depend on fontawesome-common-types depend on different versions of that types package).

Have I understood that much right?

If so, then yeah, I can imagine this being a problem that could come up often enough to be really annoying. Initially, I'd thought that, since fontawesome-common-types just holds types that almost never change, this problem should not arise very often. Then I was reminded, looking more closely, that it also includes that IconName type which lists every icon name, and that one changes often as we add new icons.

When you suggest:

Make IconDefinition and IconPack independent from the core icons

Could you clarify what you mean?

By "core icons", do you mean something in fontawesome-svg-core? Or icon packages like free-solid-svg-icons? Or something else?

Do you mean something like moving the type definitions for IconDefinition and IconPack out of fontawesome-common-types and into one of the other packages?

I looked through the demo code in the repo, but maybe my TypeScript knowledge is just too fuzzy to notice the key difference between the first and third commits in terms of making those two types independent from the core icons.

I don't have any pushback about it at this point--I'm still just trying to catch up with you to understand the problem and proposed solution.

[mlwilkerson]

Separately, if the problem has to do with FA versions getting out of sync between the dependencies of one package (like fontawesome-svg-core) and another (like free-solid-svg-icons), is it a possible solution to just somehow make sure that those versions don't drift out of sync?

In other words, rather than making the packages resilient to multiple instances of fontawesome-common-types is it feasible to just stop the "multiple instances" scenario from happening?

Is there any compelling reason a developer might intend to use package versions that are out of sync in this way? Or would it (almost) always be the case that this drift would be a mistake or an oversight? Would it (almost) always be the case that getting the FA versions in sync would produce the equivalent intended results for the developer while also avoiding this type incompatibility problem?

[devoto13]

Have I understood that much right?

Yes, pretty much. Basically every time one updates an icon package, they have to also update fontawesome-svg-core, otherwise they will not be able to add any icon to the library, because IconDefinition (which is expected by the core) does not allow new icon names.

With the proposed approach it is still not perfect as users still have to update to the latest fontawesome-svg-core to be able to use new icons, but at least the update itself will not produce type errors.

Could you clarify what you mean?

The key change is in fontawesome-common-types/index.d.ts.

Before IconDefinition had prefix and iconName set to corresponding type unions, while now they are set to string, which makes IconDefinition types from different versions of fontawesome-common-types compatible with each other as long as the shape of the icon definitions is unchanged. This is the main part. Ideally we would also split regular and duotone icon definitions into separate types.

Do you mean something like moving the type definitions for IconDefinition and IconPack out of fontawesome-common-types and into one of the other packages?

This is not required, but can be done as a follow up. E.g. we can inline IconDefinition and IconPack into icon packages, so icon package does not depend on commont-types. Once this is done fontawesome-svg-core will be the only consumer of common-types, so it make sense to include types there, effectively eliminating common-types package. But I'm not sure if this makes sense, maybe there are other relevant usages for it in FA ecosystem.

Separately, if the problem has to do with FA versions getting out of sync between the dependencies of one package (like fontawesome-svg-core) and another (like free-solid-svg-icons), is it a possible solution to just somehow make sure that those versions don't drift out of sync?

In other words, rather than making the packages resilient to multiple instances of fontawesome-common-types is it feasible to just stop the "multiple instances" scenario from happening?

Is there any compelling reason a developer might intend to use package versions that are out of sync in this way? Or would it (almost) always be the case that this drift would be a mistake or an oversight? Would it (almost) always be the case that getting the FA versions in sync would produce the equivalent intended results for the developer while also avoiding this type incompatibility problem?

It is indeed a possible solution. And normally user would want to have all these versions in sync. On the other hand it will require user to be more involved, especially with the current versioning... We can make commont-types a peer dependency of the application (requiring user to keep track of the versions and update all of them in sync). Alternatively we can do some other options with peer dependencies, but IMO this still requires user to do the right thing and does not really improve the experience.

In the original issue user didn't update FA package, but added another icon package with the newer version (without even thinking about it, just npm i @fortawesome/free-regular-svg-icons) and got the error trying to add icon from the newly installed package.

I'm also a bit biased here, because I want to get closer to solution for FortAwesome/angular-fontawesome#172 and FortAwesome/angular-fontawesome#230.

[mlwilkerson]

The key change is in fontawesome-common-types/index.d.ts.

Oh, I see it now! Thank you. GitHub had kinda put that key difference in my blind spot by greying it out by default in the diffs:

Right, so now it makes sense to me how this relaxing, from a union type of discrete string values, to more general string, makes this a duck typing solution.

And I think I get how this could come at a trade-off with completion. By not listing all of the discrete possibilities for the values in those types, a code editor wouldn't know what icon name or prefix options to make available in some cases, right?

[devoto13]

Well, this particular relaxing in IconDefinition is fine on its own, because it does not really add any safety to restrict the definition of the icon. Kinda checking that true === true. More strict type also complicates custom icons implementation.

E.g. one has to do this:

export const faDummy: IconDefinition = {
  prefix: 'fac' as IconPrefix, // pointless cast
  iconName: 'dummy' as IconName, // pointless cast
  icon: [512, 512, [], 'f030', ['M50 50 H412 V250 H50 Z', 'M50 262 H412 V462 H50 Z']],
};

instead of this:

export const faDummy: IconDefinition = {
  prefix: 'fac',
  iconName: 'dummy',
  icon: [512, 512, [], 'f030', ['M50 50 H412 V250 H50 Z', 'M50 262 H412 V462 H50 Z']],
};

library.add() does not benefit from the more restrictive type either. You normally would pass an object there and can pick any name for your (custom) icon.

The problem comes from the feature of TypeScript, where it simplifies types - microsoft/TypeScript#29729.

icon() function from the svg-core can accept both IconLookup and IconDefinition, but with this change IconLookup | IconDefinition is simplified to {iconName: string, prefix: string, icon?: [...]}, so icon({prefix: 'fab', iconName: 'user'} usage looses the completion.

[robmadole]

I'd be interested to hear from more TypeScript users on this. The in-editor completion seems to be the only possible objection here. Everything else about this change seems to be an overall net gain.

I'm trying to think about how we can target some of our TS users directly without spamming a larger list. @mlwilkerson you have any thoughts?

[devoto13]

One way to avoid losing the completion may be to use LiteralUnion. I remember that this approach had some tricky corner cases and is essentially a hack. I bit overloaded at work these days, but I'll try to squeeze some time in the next week or two to experiment with it and see if it will work here.

[devoto13]

Hey @robmadole @mlwilkerson,

There was a new problem caused by this issue reported recently in FortAwesome/angular-fontawesome#125 (comment). The problem is that the user used the SVG Core 1.3.x line with the icon packs of the FontAwesome 5, which are incompatible as shown by this minimal reproduction.

As we're not able to find a feasible solution to support multiple versions of the @fortawesome/fontawesome-common-types so far, here is another proposal.

Please consider aligning the versions of @fortawesome/fontawesome-common-types and @fortawesome/fontawesome-svg-core with the main FontAwesome releases, e.g. use the same versioning as for the icon packages themselves so that we can tell users to maintain all package versions the same and this way prevent them from getting into the situation where they have multiple conflicting type definitions.

If we do this and also specify the fixed version of the @fortawesome/fontawesome-common-types dependency everywhere we have a very simple heuristic for the users - all versions should be the same and then they will always have a single instance of the common types matching the version of the icon packages they use and therefore having the correct types without any drawbacks.

The current situation where icon packages, common types, and SVG core have their own version makes it very hard to explain to the user what they need to do to solve the issue and in some cases makes it even impossible because of dependencies like "@fortawesome/fontawesome-common-types": "^0.2.36" in the tree, which user can't do anything about without untrivial manipulations with the lock file or other fragile workarounds.

I expect that this will become a very common issue for all TypeScript users moving forward specifically because of the @fortawesome/fontawesome-svg-core package, which was released with the breaking changes, but as a minor version. There are A LOT of projects in the wild that set it as ^1.2.14 (note ^), which will automatically install 1.3.x line because it is compatible with the SemVer range.

I think we should publish @fortawesome/fontawesome-svg-core 6.0.0 and @fortawesome/fontawesome-common-types 6.0.0, then mark @fortawesome/fontawesome-svg-core 1.3.0 and @fortawesome/fontawesome-common-types 0.3.0 as deprecated, so that package managers will avoid these versions when resolving ^1.2.14 range, but will keep these version published to avoid breaking people who have already installed them.

[mlwilkerson]

Thank you, @devoto13. This seems reasonable to me too. I did spend some time investigating a related issue recently for someone facing this very challenge--the root cause of which had to do with the version of @fortawesome/fontawesome-common-types. So I appreciate the frustration that this solution would alleviate.

I've raised the concern internally as well, intending to get a plan in place for resolution.

[robmadole]

@devoto13 yep, I agree with @mlwilkerson. Your plan sounds correct and getting this fixed is high on the priority list. This is a mess and we'll look to get is solved as soon as we can. This will just continue to generate heartache and work for everyone.

[devoto13]

I'm going to close this issue as the above solution has been implemented.