JedWatson / classnames

A simple javascript utility for conditionally joining classNames together
MIT License
17.59k stars 561 forks source link

fix: new @types/react support #392

Closed Netail closed 6 months ago

Netail commented 6 months ago

Support new @types/react version (v18.2.78 and higher)

jonkoops commented 6 months ago

Could you elaborate why this is needed? If you don't provide a reproducible example of the bug then it's hard to judge if these changes are needed.

Netail commented 6 months ago

Could you elaborate why this is needed? If you don't provide a reproducible example of the bug then it's hard to judge if these changes are needed.

@types/react added bigint support since v18.2.78, which currently seems to break typescript when building;

 Type error: Argument of type '[string, string | false | 0 | 0n | null | undefined, string | undefined, string | false | undefined, string | undefined]' is not assignable to parameter of type 'ArgumentArray'.
   Type 'string | false | 0 | 0n | null | undefined' is not assignable to type 'Argument'.
     Type '0n' is not assignable to type 'Argument'.

   75 | }) => {
   76 |     const classes = classnames(
 > 77 |         children && css.hasChildren,
      |         ^

as classnames does not support this type. Same issue with clsx; https://github.com/lukeed/clsx/pull/96

jonkoops commented 6 months ago

I don't think we ever explicitly supported passing in a ReactNode, what would be the expectation of that? Could you provide a minimal example of passing a ReactNode in a practical use-case?

Netail commented 6 months ago

I don't think we ever explicitly supported passing in a ReactNode, what would be the expectation of that? Could you provide a minimal example of passing a ReactNode in a practical use-case?

For example if you want change some css based on if the children are passed into the component;

import type { FC, PropsWithChildren } from 'react';
import classNames from 'classnames';
import css from './test.module.scss';

export const Test: FC<PropsWithChildren> = ({ children, className }) => {
    const classes = classNames(
        css.root,
        children && css.hasChildren,
        className,
    );

    return (
        <div className={classes}>
            <h1>Hello</h1>
            {children}
        </div>
    )
}
jonkoops commented 6 months ago

Thanks, I see your point. Since we have supported this historically I think we should land this, however this should be redirected to the v2 branch, as we intend a different API for v3 in the future, and it will likely not be released for a while.

Netail commented 6 months ago

Thanks, I see your point. Since we have supported this historically I think we should land this, however this should be redirected to the v2 branch, as we intend a different API for v3 in the future, and it will likely not be released for a while.

You can merge it into main and then cherry pick this change into the v2 branch, maybe? Cuz the main also has the wrong typings

jonkoops commented 6 months ago

No, this has to go into the v2 branch. The typings are correct for v3, and permutations there are likely to be reduced even further.

Netail commented 6 months ago

No, this has to go into the v2 branch. The typings are correct for v3, and permutations there are likely to be reduced even further.

Will create a new PR for that :) As changing the target branch pulls the commits in