jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
9k stars 2.77k forks source link

[react/require-default-props] False positive with React.forwardRef #2856

Open nwalters512 opened 3 years ago

nwalters512 commented 3 years ago

The react/require-default-props rule configured with ignoreFunctionalComponents: true produces false positives for functional components that use React.forwardRef, like the following:

import React from "react"
import PropTypes from "prop-types"

const Demo = React.forwardRef((props, ref) => {
  const { text } = props
  return <div ref={ref}>{text}</div>
})

Demo.propTypes = {
  // Error: propType "text" is not required, but has no corresponding defaultProps declaration
  text: PropTypes.string,
}

export default Demo

Here are two CodeSandbox links demonstrating the problem with both .jsx and .tsx files:

Run yarn lint to reproduce.

I would expect components built with React.forwardRef to be considered functional components and be ignored when ignoreFunctionalComponents is true.

ljharb commented 3 years ago

Indeed, forwardRef does not take a component - in that case, that's just a normal function.

However, in this case, the warning is 100% correct. You provided a non-required propType, without a defaultProp. The point of the rule is to error on that.

Note that functional components accept propTypes, defaultProps, contextTypes, etc - almost every static property that class components support, functional components do.

nwalters512 commented 3 years ago

But Demo itself (the return value of React.forwardRef) is a React component, correct? And given that it's certainly not a class component, I would expect to not get an error if I tell the rule to ignore functional components. Does a React.forwardRef component exist in a limbo between true functional components and class components such that ignoreFunctionalComponents does not apply here?

ljharb commented 3 years ago

Demo itself is not, it's a forwardRef component, which is a distinct and unique thing - and EVERY component, class or function, needs both propTypes and defaultProps as appropriate. With ignoreFunctionalComponents, I would expect only unwrapped functional components to be ignored.

nwalters512 commented 3 years ago

Interesting! In that case, would you accept a PR adding something like ignoreForwardRefComponents to complement ignoreFunctionalComponents?

ljharb commented 3 years ago

Since there's forwardRef components, memo components, lazy components, I don't think we'd want to add one option for each kind.

I'm curious about the exact use case here - why don't you want defaultProps on a non-required component here, but you do want that error on class components?

nwalters512 commented 3 years ago

For the same reason we don't want to require defaultProps on "standard" functional components - we want to allow developers to use destructuring with defaults if they prefer. I know the debate over that has been had many times before in this repo, so I don't particularly want to rehash it, but suffice it to say that we've made the call to allow it and have configured this lint rule as such. forwardRef components might not be strictly the same as functional components, but they still use a render function, can use hooks, and can be written easily with destructuring defaults.

As to "why enable it for class components", we'd like to keep the rule active for our legacy class components since one can't use destructuring as easily, i.e. you'd need to include the same defaults every time you access this.props in a class method. We write virtually no class components now, but as long as they exist in our codebase, we'd like to apply this lint rule to them.

ljharb commented 3 years ago

The only reasonably justification that's been offered for omitting defaultProps on functional components is if you're using a type system that can infer the same information.

In this case, what does TypeScript or Flow infer for the type of text inside the non-component render function that's passed to forwardRef?

nwalters512 commented 3 years ago

That's actually exactly my use case - we're using TypeScript!

Here's a slightly more real-world example:

import React from "react"
import PropTypes from "prop-types"

interface DemoProps {
  text?: string
}

const Demo = React.forwardRef<any, DemoProps>((props, ref) => {
  const { text } = props
  return <div ref={ref}>{text}</div>
})

Demo.propTypes = {
  text: PropTypes.string,
}

export default Demo

Now that we've explicitly typed the prop text as string | undefined (via text?: string), the variable text is typed as such in the body of the forwardRef render function. So, as written, adding something like console.log(text.substring(1)) in the body of the render function would fail with a type error: Object is possibly 'undefined'.ts(2532).

Now consider the case where we provide a default when destructuring:

const Demo = React.forwardRef<any, DemoProps>((props, ref) => {
  const { text = 'default text' } = props
  return <div ref={ref}>{text}</div>
})

Even though the prop text is typed as string | undefined, the destructured variable text has the type string, since it will assume the default value if the prop isn't defined.

ljharb commented 3 years ago

The issue is that react components (like functions) actually have two type signatures: the external one, that consumers/callers see, and the internal one, that the component/function body sees. In your case, DemoProps is the external one, where text is optional. It's an implementation detail of the body that you handle the case where it's not provided.

Without a type system, this would be modeled with an optional propType and a defaultProp.

I do agree that require-default-props should allow, at least, the destructuring of props in the forwardRef function signature, and this seems reasonable as default behavior.

therealgilles commented 2 years ago

Here is what I ended up doing to workaround the issue:

import React from 'react'
import type { ForwardedRef } from 'react'

type CompProps = {
  propRequired: string
  propOptional?: string
}

const defaultProps = {
  propOptional: 'default string', // or undefined
}

// Specify underlying component and attributes types
// Different components will have different attributes getters.
type UnderlyingComp = HTMLDivElement
type UnderlyingCompAttributes = React.HTMLAttributes<HTMLDivElement>

// Exclude component props in case there is an overlap
type OtherProps = Omit<
  React.PropsWithoutRef<UnderlyingCompAttributes>,
  'propRequired' | 'propOptional'
>

type CompPropsOpt = CompProps & OtherProps
type CompPropsReq = Required<CompProps> & OtherProps

const Comp = (props: CompPropsReq, ref: ForwardedRef<UnderlyingComp>) => {
  const { propRequired, propOptional, children, ...otherProps } = props

  // ... do something with propRequired & propOptional ...

  return (
     <div ref={ref} {...otherProps}>{children}</div>
  )
}

// Call forwardRef with required props type but cast with optional props type
const CompWithForwardedRef = React.forwardRef<UnderlyingComp, CompPropsReq>(
  Comp
) as React.ForwardRefExoticComponent<CompPropsOpt>

// Add default props and display name
CompWithForwardedRef.defaultProps = defaultProps
CompWithForwardedRef.displayName = 'Component'

export default CompWithForwardedRef
bryanltobing commented 2 years ago

is there a workaround for this?

cristian-atehortua commented 2 years ago

Hi, I have a similar case.

I'm using "react/require-default-props": ["error", { functions: "defaultArguments" }] and I have a component similar to this

import React, {  forwardRef, memo } from 'react';
import PropTypes from 'prop-types';

const Button = forwardRef(({
  children,
  onClick = () => { console.log('Button clicked') },
}, ref) => (
  <button
    type="button"
    onClick={onClick}
    ref={ref}
  >
    {content}
  </button>
));

Button.propTypes = {
  children: PropTypes.node.isRequired,
  onClick: PropTypes.func,
};

export default memo(Button);

What I expected here is to not have Eslint errors since we are specifying default props in the function arguments as is configured. However, react eslint plugin is not detecting this as a function component because of the forwardRef and enforces me to define Button.defaultProps.

Can this be solved?

ljharb commented 2 years ago

The rule is called "require default props", and .defaultProps is superior to default arguments for a number of reasons.

In this case, your default onClick callback is created anew every render, without useCallback, where with defaultProps it would not be, so using default arguments here is a potential performance hit unless you remember to wrap it in useCallback (altho in this case, you're passing it to an HTML element, so it's fine, but it's still better to memoize all objects passed as props as good hygiene)

Either way, https://github.com/jsx-eslint/eslint-plugin-react/issues/2856#issuecomment-747742498 and the "help wanted" label still apply, so if you want it solved, submit a PR.

cristian-atehortua commented 2 years ago

@ljharb Take a look on this:

React is deprecating defaultProps on function components.

ljharb commented 2 years ago

I'm aware, but they're wrong to do.

Separately, that RFC is from 2019, and despite two major versions being released since then, they haven't taken steps to go further.

SalahAdDin commented 1 year ago

I am having the same problem but without using any forwardRef.

woodreamz commented 1 year ago

So forwardRef and "functions": "defaultArguments" are just incompatible? There is no workaround?

vicasas commented 11 months ago

Same problem here 🤚

chandra-logituit commented 11 months ago

@cristian-atehortua did you find any solution/workaround for this?

cristian-atehortua commented 11 months ago

Hi @chandra-logituit. You have two options:

const Button = forwardRef(({ children, onClick, }, ref) => ( <button type="button" onClick={onClick} ref={ref}

{content} ));

Button.defaultProps = { onClick: () => { console.log('Button clicked') }, };

Button.propTypes = { children: PropTypes.node.isRequired, onClick: PropTypes.func, };

export default memo(Button);


 or,
- define the component as a function and wrap it in forwardRef in a different clause (ugly)
```js
import React, {  forwardRef, memo } from 'react';
import PropTypes from 'prop-types';

const Button =({
  children,
  onClick = () => { console.log('Button clicked') },
}, ref) => (
  <button
    type="button"
    onClick={onClick}
    ref={ref}
  >
    {content}
  </button>
);

Button.propTypes = {
  children: PropTypes.node.isRequired,
  onClick: PropTypes.func,
};

export default memo(forwardRef(Button));
chandra-logituit commented 11 months ago

@cristian-atehortua Thank you so much for taking your time to explain it. I am familiar with both approaches, but my question was related to the false positive with forwardRef. Anyways we have decided to go with defaultProps which is working perfectly fine.

cristian-atehortua commented 11 months ago

HI @chandra-logituit. Yes, they are the only workarounds I know to face the issue with the false positives when using forwardRef.

ololoepepe commented 2 months ago

defaultProps are deprecated. You can see a warning in browsers for React.memo: Support for defaultProps will be removed from memo components in a future major release. Use JavaScript default parameters instead. Default function parameters MUST be supported for memo/forwardRef/etc. instead of setting defaultProps.

SalahAdDin commented 2 months ago

defaultProps are deprecated. You can see a warning in browsers for React.memo: Support for defaultProps will be removed from memo components in a future major release. Use JavaScript default parameters instead. Default function parameters MUST be supported for memo/forwardRef/etc. instead of setting defaultProps.

Yet there is no solution for this issue yet.

ljharb commented 2 months ago

The solution is to disable this rule entirely - why would you need it in react 19+?

SalahAdDin commented 2 months ago

The solution is to disable this rule entirely - why would you need it in react 19+?

what if you use javascript?

ljharb commented 2 months ago

This rule is for requiring .defaultProps on react components, and it only has relevance in that context. What does JS have to do with it?

cristian-atehortua commented 2 months ago

But you have the default arguments form of this rule. With this rule using the arguments form you can force to define a default value for props that are not marked as required in the proptypes.

Probably a better approach will be to make the default arguments as the default (and probably only) way for function components (and memo, and forwardRef)

Another point, aren't the defaulProps still supported for class components?

ljharb commented 2 months ago

class components

Sure, but react doesn't warn for those. Good point tho that the rule continues to have value in React 19+!

as required

.propTypes is also removed in React 19, so at that point, you're using TS types, and I believe that ensures that default arguments match the types for SFCs.

Changing the default is a breaking change, unfortunately, so that's not an option.