tools icon indicating copy to clipboard operation
tools copied to clipboard

💭 Suppress noPropSpreading if using TypeScript?

Open caseyjhol opened this issue 3 years ago • 2 comments

I'm using Material UI and trying to customize some of the components. So, I'm wrapping them in a new component and using prop spreading to pass the props down. I'm also using TypeScript, and I'm importing the components' associated interfaces from Material. This is triggering lint/jsx/noPropSpreading, but are there really any downsides to prop spreading when using it in conjunction with TypeScript interfaces (i.e allowable props are already defined)?

Thanks!

import {
	Button as MaterialButton,
        ButtonProps
} from "@material-ui/core";

export function Button(props: ButtonProps) {
	return (
                <MaterialButton {...props}>
		        {props.children}
	        </MaterialButton>
        )
}

caseyjhol avatar Aug 28 '20 16:08 caseyjhol

I think the lint rule reasoning still applies, it's not clear what properties you actually care about. Maybe it's appropriate here to recommend a suppression. There might be another way we can balance this though since there are places where you're using types and just want to pass everything in.

I think the case you mention is common though. Wrapping another component you don't necessarily care about it since it's an implementation detail.

I wonder if there is a heuristic we can add to make this better and avoid the error. I don't think just being in TypeScript is good enough. Maybe this is just one of those things where we should remove it until we have access to type information to emit it based on the amount of props, where they're defined (in your app code or a library or definition) etc.

sebmck avatar Aug 30 '20 20:08 sebmck

Two things:

  • Since we are now talking about configuration in Rome, I think this is a lint rule we wouldn't put in the recommended set as it highly-opinionated against what most of the community does
  • If we can integrate type information, we should modify this rule to require an "exact" object type when rest is used.
    • Alternatively: This could be fixed with improvements to TypeScript and we could potentially work with them to introduce some sort of exact object type or a new strict* option

To summarize though, the issue is that you can pass additional props into a component that may later become part of the component's API:

interface ComponentProps {
    foo: boolean,
    bar: boolean,
    baz: boolean,
    // what happens if I add `bat: boolean` but it means something different than the other `bat` being passed in
}

function Component(props: ComponentProps) {
    // ...
}

let rest = {
    bar: true,
    baz: true,
    bat: true,
}

let a = <Component foo={true} {...rest} />

jamiebuilds avatar Jun 09 '21 21:06 jamiebuilds