Allow passing undefined to optional properties with exactOptionalPropertyTypes

[jdufresne]

The TypeScript configuration option exactOptionalPropertyTypes is documented at:

https://www.typescriptlang.org/tsconfig/#exactOptionalPropertyTypes

With exactOptionalPropertyTypes enabled, TypeScript applies stricter
rules around how it handles properties on type or interfaces which
have a ? prefix.

This means TypeScript will handle "undefined" differently than "not provided". Consider the following construct:

function MyIcon({ addCLass: false }) {
  let className: string | undefined;
  if (addClass) {
    className = "mx-2";
  }
  return <FontAwesomeIcon icon={["fal", "coffee"]} className={className}/>
}

In this scenario, classname will sometimes be passed as "undefined". This is allowed by the implementation, so allow it through the type interface as well.

[charles4221]

It seems like overkill to me to update the library to use className?: string | undefined just for this one property for one specific use case. We're already marking the property as optional and capable of receiving undefined by using the ? prefix.

If your project has exactOptionalPropertyTypes enabled, you're overriding that behaviour and telling TypeScript that you want to be warned not to pass undefined to some property that takes an optional string. So instead of changing this library to avoid having to do that in your project, you could turn that option off, or you could follow its premise and simply default your className to an empty string rather than undefined.

E.g.

let className = '';
if (addClass) {
  className = "mx-2";
}

[jdufresne]

The configuration option exactOptionalPropertyTypes is used by the React project itself for their type definitions. As part of the React ecosystem, I believe this project should too to follow community conventions and best practices.

See https://github.com/DefinitelyTyped/DefinitelyTyped/blob/183d197bc71cb0aa4ccc22276381ed3853fa22e1/types/react/tsconfig.json#L24

It seems like overkill to me ... just for this one property

We're talking about one line to allow the property to be undefined. I feel like "overkill" is exaggerating the change here.

However, if you're aware of other properties that require the same change, I'm happy to include them here within the same change. So far, I'm not aware of any.

[charles4221]

I do understand what you’re saying. However, the property is already allowed to be undefined, by being marked as optional. The type evaluates as “string | undefined” already. The typescript setting you’re using is specifically designed to override that behaviour. By enabling it, you’re saying you want this alternate behaviour. Libraries that you consume should not need to be changed to avoid having to follow the rules of your project configuration. You could resolve this issue for yourself instantly by replacing the default undefined with an empty string, as per the rules of the exactOptionalPropertyTypes setting.

If you don’t want to have to do that, then I have to wonder why you have the TS configuration enabled at all? (Not trying to be snarky, just genuinely curious)

[jdufresne]

However, the property is already allowed to be undefined, by being marked as optional. The type evaluates as “string | undefined” already.

The property is defined as optional, yes. This means it can be omitted. However, it is not defined as allowing undefined. To do that, we require this change provided here. Now, it is true, that the default TypeScript configuration mixes the two concepts into one until exactOptionalPropertyTypes is enabled, but as a community friendly library, we can easily accommodate both configuration by accepting this change.

Libraries that you consume should not need to be changed to avoid having to follow the rules of your project configuration.

FWIW, nearly all the libraries I have come in contact with adjust for this TypeScript configuration option already. Right now, react-fontawesome is the only library that still does not for me and my project. I think if you were to survey other big libraries, you'll find most define their types for compatibility with exactOptionalPropertyTypes.

You could resolve this issue for yourself instantly by replacing the default undefined with an empty string, as per the rules of the exactOptionalPropertyTypes setting.

I have already worked around the issue within my own codebase. You provided an option, but there are others as well. I'm here to try to improve react-fontawesome so other library consumers don't need to apply the same workarounds to bypass this false-error.

If you don’t want to have to do that, then I have to wonder why you have the TS configuration enabled at all? (Not trying to be snarky, just genuinely curious)

I have this option enabled because it strengthens the TypeScript type system in way that is similar to strict and its associated options. It strengthens the type system by making a distinction between "omitted" and undefined. These different states can sometimes have real consequences in programs such that their definition ought to be deliberate. It just so happens that these two states don't make a difference for className in react-fontawesome, but that isn't consistently true for all TypeScript. The fact that these two states don't make a difference in react-fontawesome means that we should accept this change.

Thanks for the responses and for considering.

[charles4221]

@jdufresne very fair call mate, thanks for walking through it in detail! Given our discussion, I'm keen for this to go ahead, but would you be able to expand upon the PR so all optional props on FontAwesomeIconProps have this same provision? We don't have any distinct logic between "omitted" and undefined within this library, so if we add it for className then it makes sense to add it to all of the optionals imo.

[jdufresne]

That makes sense to me. I updated the PR to to apply the same change to all optional properties.