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/prop-types - false positive when returning null #2703

Closed
gcourtemanche opened this issue Jul 8, 2020 · 20 comments · Fixed by #2721
Closed

[BUG] react/prop-types - false positive when returning null #2703

gcourtemanche opened this issue Jul 8, 2020 · 20 comments · Fixed by #2721

Comments

@gcourtemanche
Copy link

I'm trying to update 7.20.0 to 7.20.3, but I have new false positive errors for components that can return null.

This is telling me 'number' is missing in props validationeslintreact/prop-types:

import React from 'react';

type Props = {
  number: number;
};

export default (props: Props) => {
  const { number } = props;

  if (number === 0) {
    return <p>Number is 0</p>;
  }

  return null;
};

But this is fine:

import React from 'react';

type Props = {
  number: number;
};

export default (props: Props) => {
  const { number } = props;

  if (number === 0) {
    return <p>Number is 0</p>;
  }

  return <p>Number is not 0</p>;
};
@ulrik59
Copy link

ulrik59 commented Jul 8, 2020

Same problem for me

@ljharb
Copy link
Member

ljharb commented Jul 8, 2020

I assume that if you flip the first example so the null is returned conditionally and the jsx unconditionally, that it’ll be detected as a component properly?

@gcourtemanche
Copy link
Author

@ljharb exactly, no issue if the null is returned in the condition

@ljharb
Copy link
Member

ljharb commented Jul 8, 2020

Specifically, component detection should be looking at every return value, and if any of them are jsx, and if all of them are possibly valid component return values (jsx, null, or a string/number/array of those things), then it should be detected as a component. (cc @alexzherdev)

@hank121314
Copy link
Contributor

Hi @gcourtemanche !
I have made a branch trying to resolved this problem.
Could you try this branch https://github.com/hank121314/eslint-plugin-react/tree/develop and test whether the issue is resolved?
Thanks!

@gcourtemanche
Copy link
Author

Hi @hank121314, thanks for helping me with this issue. I tried running eslint using commit ad0bb3005640eb7f147916e6b524e61be2e71992 and I got this error:

Oops! Something went wrong! :(

ESLint: 7.3.1

TypeError: Cannot read property 'forEach' of undefined

@hank121314
Copy link
Contributor

hmm, sorry for that.
Could you please give me your example code and which parser did you use?
Thanks!

@gcourtemanche
Copy link
Author

With @typescript-eslint/parser, any file is throwing the error.

image

@hank121314
Copy link
Contributor

If you don’t mind, could you please give me full error code?
And I need the file which occurred.
If any file is throwing the error, please give me an example code.
Like this.

Oops! Something went wrong! :(

ESLint: 7.3.1

TypeError: Cannot read property 'heritage' of undefined
Occurred while linting /Users/hankchen/Desktop/FrontEnd/Electron/electron-react-template/src/app/components/Charts/GaugeChart/GaugeChart.tsx:32← this file.
    at declarePropTypesForTSTypeAnnotation (/Users/hankchen/Desktop/FrontEnd/Electron/electron-react-template/node_modules/eslint-plugin-react/lib/util/propTypes.js:465:56)
    at markPropTypesAsDeclared (/Users/hankchen/Desktop/FrontEnd/Electron/electron-react-template/node_modules/eslint-plugin-react/lib/util/propTypes.js:677:33)
    at Object.markAnnotatedFunctionArgumentsAsDeclared (/Users/hankchen/Desktop/FrontEnd/Electron/electron-react-template/node_modules/eslint-plugin-react/lib/util/propTypes.js:720:7)
    at updatedRuleInstructions.<computed> (/Users/hankchen/Desktop/FrontEnd/Electron/electron-react-template/node_modules/eslint-plugin-react/lib/util/Components.js:914:43)
    at /Users/hankchen/.config/yarn/global/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/Users/hankchen/.config/yarn/global/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/Users/hankchen/.config/yarn/global/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (/Users/hankchen/.config/yarn/global/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode (/Users/hankchen/.config/yarn/global/node_modules/eslint/lib/linter/node-event-generator.js:297:14)

If the occurred file is private you can mail to my email(hank121314@gmail.com).
Thanks you very much @gcourtemanche!

@hank121314
Copy link
Contributor

Hi @gcourtemanche !
I think I might have found the issue related.
Did your repository have some syntax Intersection Types:
Like this:

type User = {
  user: string;
 }
// This line
type Props = User & {
  userId: string;
};

export default (props: Props) => {
  const { userId, user } = props;
  if (userId === 0) {
    return <p>userId is 0</p>;
  }
  return null;
 };

@gcourtemanche
Copy link
Author

@hank121314 yes, it does have intersection types!

@hank121314
Copy link
Contributor

The latest commit at developer branch solved intersection types issue! Please try it.
Thanks!

@gcourtemanche
Copy link
Author

@hank121314 I can't find the developer branch :(

@hank121314
Copy link
Contributor

Forget to tell I commit to my own fork. Here:https://github.com/hank121314/eslint-plugin-react/tree/develop

@gcourtemanche
Copy link
Author

The error is gone but another error appeared:
propType "number" is not required, but has no corresponding defaultProps declaration. eslintreact/require-default-props

Using this code:

import React from 'react';

type Props = {
  number?: number;
};

export default (props: Props) => {
  const { number } = props;

  if (number === 0) {
    return <p>Number is 0</p>;
  }

  return null;
};

If I remove the optional to number, there is no error anymore.

@hank121314
Copy link
Contributor

This error appeared because of rule(require-default-props).
Try to define defaultProps for react function component.
microsoft/TypeScript#31247

@gcourtemanche
Copy link
Author

@hank121314 I only get the error when I'm using your branch

@hank121314
Copy link
Contributor

The error should display.
The reasons why 7.20.3 won't display are:

  1. 7.20.3 does not support TSTypeAliasDeclaration(like: type Props = { user: string; }) yet.(Fixed in my develop branch)
  2. 7.20.3 will get some bug when conditionally return JSXElement.(Fixed in pr: [Fix]:prop-types: handle component returning null #2696)
    Running the code below, it should show the same error.
interface Props {
  number?: number;
};
export default (props: Props) => {
  const { number } = props;
  if (number === 0) {
    return null;
  }
  return <p>Number is 0</p>;
}

@gcourtemanche
Copy link
Author

Great, thank you for you help @hank121314 ! :)

@hank121314
Copy link
Contributor

Anytime 😄 .
I will make a pr after my latest pr is merged

ljharb pushed a commit to hank121314/eslint-plugin-react that referenced this issue Jul 31, 2020
…ript props interface extension and TSTypeAliasDeclaration

Fixes jsx-eslint#2654. Fixes jsx-eslint#2719. Fixes jsx-eslint#2703.
ljharb pushed a commit to hank121314/eslint-plugin-react that referenced this issue Jul 31, 2020
…ript props interface extension and TSTypeAliasDeclaration

Fixes jsx-eslint#2654. Fixes jsx-eslint#2719. Fixes jsx-eslint#2703.
ljharb pushed a commit to hank121314/eslint-plugin-react that referenced this issue Jul 31, 2020
…ript props interface extension and TSTypeAliasDeclaration

Fixes jsx-eslint#2654. Fixes jsx-eslint#2719. Fixes jsx-eslint#2703.
@ljharb ljharb closed this as completed in 90d2cdf Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants