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

[Bug]: react/prefer-read-only-props doesn't work with implicit React reference (react/jsx-runtime) #3650

Open
2 tasks done
alexilyaev opened this issue Nov 12, 2023 · 8 comments

Comments

@alexilyaev
Copy link
Contributor

alexilyaev commented Nov 12, 2023

Is there an existing issue for this?

  • I have searched the existing issues and my issue is unique
  • My issue appears in the command-line and not only in the text editor

Description Overview

Adding react/prefer-read-only-props rule does not trigger when using React.FC or React.FunctionComponent without importing React.

In modern React, we don't need to import React thanks to the new JSX Transform:

And TypeScript supports this out of the box:

So even for React types we don't have to import them. React.FC just works in .tsx files.

But react/prefer-read-only-props fails to identify the component as a React component if we use React.FC or React.FunctionComponent without importing React.

Expected Behavior

This code does NOT error

interface ChipProps {
  chipColor: string;
  label: string;
}

const Chip: React.FC<ChipProps> = ({ chipColor, label }) => {
  return <div style={{ backgroundColor: chipColor }}>{label}</div>;
};
image

This code does

// This line is added
import React from 'react'

interface ChipProps {
  chipColor: string;
  label: string;
}

const Chip: React.FC<ChipProps> = ({ chipColor, label }) => {
  return <div style={{ backgroundColor: chipColor }}>{label}</div>;
};
image

.eslintrc.js

module.exports = {
  root: true,
  env: {
    browser: true,
    es6: true,
    es2021: true,
  },
  plugins: ['@typescript-eslint', 'react'],
  parser: '@typescript-eslint/parser',
  parserOptions: {
    ecmaFeatures: {
      jsx: true,
    },
    ecmaVersion: 2021,
    sourceType: 'module',
  },
  settings: {
    react: {
      version: 'detect',
    },
  },
  rules: {
    'react/prefer-read-only-props': 'warn',
  },
};

tsconfig.json

{
  "compilerOptions": {
    "allowJs": true,
    "allowSyntheticDefaultImports": true,
    "baseUrl": "./",
    "composite": false,
    "declaration": false,
    "declarationMap": false,
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "isolatedModules": true,
    "jsx": "react-jsx",
    "lib": ["DOM", "DOM.Iterable", "ESNext"],
    "module": "ESNext",
    "moduleResolution": "node",
    "noEmit": true,
    "resolveJsonModule": true,
    "skipLibCheck": true,
    "strict": true,
    "target": "ES2021",
    "types": ["node", "@testing-library/jest-dom"],
  },
  "include": ["*.ts", "src/**/*.ts", "src/**/*.tsx", "src/**/*.json"],
  "exclude": ["node_modules", "build", "storybook-static"]
}

Tried in VS Code and in the command line:

yarn eslint --max-warnings 0 src/components/Chip.tsx

eslint-plugin-react version

7.33.2

eslint version

8.38.0

node version

v16.14.0

@alexilyaev alexilyaev added the bug label Nov 12, 2023
@ljharb
Copy link
Member

ljharb commented Nov 12, 2023

Oof, that seems really unfortunate from TS - referencing React without importing it is horrific. The new jsx transform works because it doesn’t transform to something that references React.

I’m not sure how we can handle this cleanly.

@alexilyaev
Copy link
Contributor Author

@ljharb Can you point me to the source logic that recognizes React.FC with the import React but that fails without the import?
Is this rule "type-aware"? Meaning can it read the types?

@ljharb
Copy link
Member

ljharb commented Nov 12, 2023

It's in lib/util/propTypes.js, and yes, it's type-aware whenever the parser provides that information.

The key is, we shouldn't be treating something as a React type unless we're certain it came from React.

@alexilyaev
Copy link
Contributor Author

alexilyaev commented Nov 13, 2023

The key is, we shouldn't be treating something as a React type unless we're certain it came from React

Even if the identifier is React.FC?
Any reason to believe it could be something that is not React?

@alexilyaev
Copy link
Contributor Author

Btw, the rule already identifies a React component even without any import matching:

interface ChipProps {
  chipColor: string;
  label: string;
}

const Chip = ({ chipColor, label }: ChipProps) => {
  return <div style={{ backgroundColor: chipColor }}>{label}</div>;
};

image

Can't we use the same heuristic to assume that when we see React.FC (without importing React) AND the function returns JSX (or however it is determined) to mean it is indeed a React component?

@ljharb
Copy link
Member

ljharb commented Nov 13, 2023

Yes, we can never assume any identifier is anything specific; someone could make their own thing called that.

a React component uses jsx syntax, and jsx-uses-react ensures that jsx syntax imports React (when not using jsx-runtime).

If it's TS that provides React.FC as an implicit global, then certainly we could look for when React.FC is used in type space, and is also NOT imported or otherwise registered as a global to eslint?

@alexilyaev
Copy link
Contributor Author

Indeed React in this pattern is provided only in type space.
This works because React with TypeScript projects install the @types/react-dom dependency (which also installs @types/react), since without these, there's a whole bunch of type errors.

image image

Considering this fact, I think it's harder to imagine someone using React.FC that is not the actual React type.
What do you think?

Side note

This issue is also relevant for react/prop-types rule, which has the same heuristic in order to disable itself:
https://github.com/vedadeepta/eslint-plugin-react/blob/master/lib/util/propTypes.js#L516 -> isValidReactGenericTypeAnnotation

And currently fails to do so with the mentioned pattern:
image

@ljharb
Copy link
Member

ljharb commented Nov 18, 2023

I'd still like to be precise about ensuring that React.FC, when React is not available, is only recognized as such when it's in type-space and React is not a global.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants