jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
8.86k stars 2.75k forks source link

[Bug]: `react/no-object-type-as-default-prop` is disabled when using a `ref` prop #3767

Closed JulienR1 closed 2 weeks ago

JulienR1 commented 2 weeks ago

Is there an existing issue for this?

Description Overview

Brief description

The react/no-object-type-as-default-prop does not run when a component has a ref prop as a second argument.

Show example of your code (as text format), add images/videos/gifs to help explain example

Both of these components should error because an object type is used as a default prop. However, the second component (with a ref) does not error.

const ErrorIsShown = ({ foo = {} }) => null;
const ErrorIsAbsent = ({ foo = {} }, ref) => null;

image

Link to repro: https://github.com/JulienR1/eslint-react-no-object-type-as-default-prop-repro

What command(s) did you run to reproduce issue?

npm run lint
# OR
eslint . --ext js,jsx --report-unused-disable-directives --max-warnings 0
// .eslintrc.cjs
module.exports = {
  root: true,
  env: { browser: true, es2020: true },
  extends: ["plugin:react/recommended"],
  ignorePatterns: ["dist", ".eslintrc.cjs"],
  parserOptions: { ecmaVersion: "latest", sourceType: "module" },
  rules: {
    "react/no-object-type-as-default-prop": "error",
    "react/prop-types": "off",
  },
};

Expected Behavior

Brief description

Components with ref props should also generate errors when using default object type props.

image

eslint-plugin-react version

v7.34.2

eslint version

v8.57.0

node version

v20.12.2

ljharb commented 2 weeks ago

That's because forwardRef's callback is not a component. Function components take context as a second argument, not ref.

JulienR1 commented 2 weeks ago

Actually, the issue does not come from the ref directly, it comes from having a second argument. If I use forwardRef as described in React's docs, I still don't get an error from eslint.

From what I can see, this behavior is caused by this line in the rule's implementation: having more than one argument in the function will not execute the rule.

Second test:

const ErrorIsShown = ({ foo = {} }) => null;
const ErrorIsAbsent = forwardRef(function({ foo = {} }, ref) {
    return null
});
ljharb commented 2 weeks ago

I see what you mean. Given that it's fine to have , context in a component, the rule should be triggered regardless of there being more than one param.

JulienR1 commented 2 weeks ago

That is exactly it. Do you want me to set up a PR fixing this?

ljharb commented 2 weeks ago

That’d be great, thanks!