typescript configuration should be more strict

[alirezamirian]

📝 Feedback

Current typescript configuration doesn't have any of the strict flags. While it might not be the biggest problem for users of @react-spectrum packages, for users of @react-aria and @react-stately, it can cause problems which are mostly related to strictNullCheck flag. A couple of examples of these problems:

• Return value of KeyboardDelegate methods should be nullable.
• In useControlledState, arguments don't allow undefined value while they should #1859

🔦 Context

We are using @react-aria and @react-stately to implement our own design system, and we have strict flag turned on in our tsconfig.

Suggestion

I suggest turning strict flag on. Or at least strictNullCheck. I would be happy to contribute by fixing current errors that would appear by strict flag. It might be hard to fix all errors at once, because of conflict, PR size, etc. It can be split into a few PRs where the change to strict flag is committed in the last one.

[LFDanLu]

This sounds reasonable to me, I'd be interested to see how much would need to be fixed.

[alirezamirian]

@LFDanLu 1673 errors with strict set to true. 1593 errors with only strictNullCheck set to true.

[dannify]

Having to add | undefined (and even null?) to pretty much every type (based on the number of errors reported for strictNullCheck) can't be the only way to solve this issue right? This seems a little over the top. Are there no other options for this in typescript?

[ChocolateLoverRaj]

@dannify One possible method could be to bulk modify every function and add | undefined to all the parameters. This wouldn't change any internal behavior, but would decrease errors in packages that use the packages.

[dannify]

This is something I would be interested in doing. We would need it on all props as well. An option I'd like to explore is to handle this at build time instead of in the source code.

[ChocolateLoverRaj]

@dannify I made a babel plugin that adds | undefined to all parameters of exported functions. Can you try that and see it it can be used during build time?

[dannify]

I think this is something we would be able to build on and use. It would also need to include all types, including Object properties, within the component packages and the shared types as well (not just function parameters).

[ChocolateLoverRaj]

K I will add object values of functions parameters too.

@dannify What do you mean by

shared types

[dannify]

A lot of our packages use the @react-types package for their types (mostly in React Aria and React Spectrum) https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/textfield/src/useTextField.ts#L13 so we would want to include those as well.

[ChocolateLoverRaj]

@dannify So I looked at https://github.com/adobe/react-spectrum/blob/main/packages/%40react-types/textfield/src/index.d.ts and it exports interfaces. So I will make the babel plugin make every property in the interface optional.

[ChocolateLoverRaj]

@dannify I released v1.2.0 of the babel plugin. Can you try it out? Let me know what other modifications the plugin needs to do.

[dannify]

Ok great! Next week we have a break but I can take a look when we are back. Thanks for the update

[unional]

Another thing to consider is enabling exactOptionalPropertyTypes.

i.e. all optional types should have | undefined.

[psirenny]

i.e. all optional types should have | undefined.

This would make it much easier to use the library in projects with exactOptionalPropertyTypes enabled. Otherwise, all undefined props passed into React Aria Components throw a type error. Workarounds are ugly. e.g.

<Checkbox
  {...(isIndeterminate === undefined ? {} : { isIndeterminate })}
>
  …
</Checkbox>

[snowystinger]

We can consider this after https://github.com/adobe/react-spectrum/issues/3183

Things that would help, a plugin to do it incrementally and a count of how many things exactOptionalPropertyTypes is affecting so we know the effort.

We also accept contributions.

[unional]

Essentially it's finding all optional parameters and slapping undefined to them.

If you don't mind putting undefined in front, it can be a find and replace of ?: => ?: undefined |

Personally, I would do a find ?: and add | undefined to the end of the type one by one. 😁