Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: new @types/react support #392

Closed
wants to merge 1 commit into from
Closed

Conversation

Netail
Copy link

@Netail Netail commented Apr 17, 2024

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

@jonkoops
Copy link
Collaborator

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
Copy link
Author

Netail commented Apr 17, 2024

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; lukeed/clsx#96

@jonkoops
Copy link
Collaborator

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
Copy link
Author

Netail commented Apr 18, 2024

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
Copy link
Collaborator

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 Netail changed the base branch from main to v2 April 20, 2024 14:32
@Netail Netail changed the base branch from v2 to main April 20, 2024 14:33
@Netail
Copy link
Author

Netail commented Apr 20, 2024

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
Copy link
Collaborator

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
Copy link
Author

Netail commented Apr 20, 2024

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

@Netail Netail closed this Apr 20, 2024
@Netail Netail deleted the fix/react-types branch April 20, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants