[Bug]: 'undefined' className added to NumberInput

[jpsorensen]

Package

@carbon/react

Browser

Chrome, Safari, Firefox, Edge

Package version

v11.19.0

React version

v17.0.2

Description

In many places throughout the code base, we apply custom classNames to child components using a pattern like:

[className]: x

where x is typically className or !!className.

In all (almost all?) these cases, className is an optional property, meaning that it will some times be undefined. This means that we will pass [undefined]: x to our classNames() function when generating the list of class names to apply to an element. In most cases, we use [className]: className, which results in passing [undefined]: undefined, which evaluates to ['undefined']: false, so the "class name" "undefined" is not added (because it is mapped to false). However, this seems unexpected and likely to cause bugs in the future.

In addition, there is one case where className can be mapped to true, in the case of NumberInput. I confirmed that in the currently released version of carbon, this means that the element actually has the string "undefined" added as a class name.

Discovered this issue while adding type annotations for #12548, but guessing this will come up in many components when we try to add type annotations (easy workaround is to cast away the | undefined using [className!], but if there's a "cleaner" solution possible that would be ideal).

Suggested Severity

Severity 4 = Unrelated to a user task, has a workaround or does not need a workaround.

Reproduction/example

https://react.carbondesignsystem.com/?path=/story/components-numberinput--default

Steps to reproduce

Open NumberInput storybook. Inspect the page and find the container div with class cds--form-item. Note that it also has a class "undefined".

Code of Conduct

• I agree to follow this project's Code of Conduct
• I checked the current issues for duplicate problems

[jpsorensen]

(I'm new to the project so please don't hesitate to let me know if there's anything I could fix up in this report to make it most useful!)

One way to address this would be to replace each instance of this with:
...(className && { [className]: className}),
so that the property is only added if className is truthy (which seems like the intent). That seems pretty verbose (and isn't great for readability) if we intend to do it across the code base any place this pattern appears.

Trying to think of whether there are better solutions.

[jpsorensen]

To make this issue more findable if others run into it while adding Typescript (as I suspect people will run into this when working on various tickets under #12513), here is the error that will appear when attempting to add Typescript annotations to a file with this issue:
A computed property name must be of type 'string', 'number', 'symbol', or 'any’.

Potential solutions:

• Assert that the variable is defined - for example, changing [customClassName]: !enabled to [customClassName!]: !enabled. This solution has the benefit that it makes no change in behavior - the type assertion is simply dropped when converted to Javascript.
• Test that the variable is defined - for example, changing [customClassName]: !enabled to ...(customClassName && {[customClassName]: !enabled}). This has the advantage of actually fixing the issue (not setting undefined as a property name), but changes the behavior (since it no longer sets undefined as a property name, and previously the code did).