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

Imported propTypes triggers false positives #2357

Closed
monsieurnebo opened this issue Jul 24, 2019 · 17 comments
Closed

Imported propTypes triggers false positives #2357

monsieurnebo opened this issue Jul 24, 2019 · 17 comments

Comments

@monsieurnebo
Copy link

monsieurnebo commented Jul 24, 2019

Hello,

Context

Package Version
eslint ~6.0.1
eslint-plugin-react ~7.14.2

The situation

After updating ESLint, I'm encountering false positives when importing propTypes (that are used in multiple places, for instance).

Let's take a simple example:

// sharedPropTypes.js

import PropTypes from "prop-types";

export default {
  steps : PropTypes.array,
};
// MyComponent.jsx

import React from "react";
import sharedPropTypes from "./sharedPropTypes";

const propTypes = {
  ...sharedPropTypes,
};

function MyComponent(props) {
  // Using steps.map somewhere
}

This scenario is triggering the following lint errors:
error 'steps.map' is missing in props validation react/prop-types

Which is new.

Please note that declaring propTypes in the component file is working fine:

// MyComponent.jsx

import React from "react";
import PropTypes from "prop-types";
import sharedPropTypes from "./sharedPropTypes";

const propTypes = {
  steps : PropTypes.array
};

function MyComponent(props) {
  // Using steps.map somewhere
}

A lead

After some searches, it seems that ESLint is more sensitive since #2301 .

  1. Could it be the source?
  2. How should I manage this situation in a clean way?

Related issues

#2352
#2343

@Borovensky
Copy link

Have the same issue.
eslint: 5.16.0
eslint-plugin-react: 7.14.3

@monsieurnebo monsieurnebo changed the title Imported propTypes fires false positives Imported propTypes triggers false positives Jul 24, 2019
@Borovensky
Copy link

Borovensky commented Jul 24, 2019

@monsieurnebo Hi!
I play a bit around it and this is what I have:
In my case I have this issue when I declaring propTypes in the component file:

// MyComponent.jsx
import React from "react";
import PropTypes from "prop-types";

const propTypes = {
  steps: PropTypes.array
};

function MyComponent(props) {
  // Using steps.map somewhere
}

but if I change the default import of propTypes to a single import from the "prop-types" module the error is gone.

// MyComponent.jsx
import React from "react";
import { array } from "prop-types";

const propTypes = {
  steps: array
};

function MyComponent(props) {
  // Using steps.map somewhere
}

Hope this will help you also.

@ljharb
Copy link
Member

ljharb commented Jul 24, 2019

You should never have to destructure off of PropTypes to have things work; that’s a bug.

The OP example with sharedPropTypes just won’t work with static analysis; I’d suggest not sharing the entire propTypes object and instead sharing only individual shapes or propTypes.

@monsieurnebo
Copy link
Author

monsieurnebo commented Jul 26, 2019

@ljharb Thanks for the answer.

Why is this error showing now, and not before? Is that related to #2301 ?

@monsieurnebo
Copy link
Author

monsieurnebo commented Jul 26, 2019

That's really strange, because I have the same situation in another project (the same props, shared between components), without the lint error...

There must be a difference, that could explain the error... But I'm struggling to find it.

EDIT: Bingo 🎉 I found the difference:

  • My initial situation triggers ESLint error when using with a functional component...
  • ... But there is no such error with a class component !

ESLint error 🚨

import React from "react";
import sharedPropTypes from "./sharedPropTypes";

const propTypes = {
  ...sharedPropTypes,
};

function MyComponent(props) {
  // Using steps.map somewhere
}

No ESLint error ✅

import React from "react";
import sharedPropTypes from "./sharedPropTypes";

class MyComponent(props) extends React.Component {
  static propTypes = {
    ...sharedPropTypes,
  }
  // Using steps.map somewhere
}

However, I have no clue of the explanation behind this behavior.

@ljharb
Copy link
Member

ljharb commented Jul 27, 2019

Where did you assign propTypes to MyComponent.propTypes in the SFC version?

@monsieurnebo
Copy link
Author

Sorry, I missed the props assignation in the pseudocode. It looks like this:

import React from "react";
import sharedPropTypes from "./sharedPropTypes";

const propTypes = {
  ...sharedPropTypes,
};

function MyComponent(props) {
  // Using steps.map somewhere
}

MyComponent.propTypes = propTypes;

@ljharb
Copy link
Member

ljharb commented Jul 30, 2019

what happens if you do MyComponent.propTypes = { ...sharedPropTypes }?

@monsieurnebo
Copy link
Author

monsieurnebo commented Jul 30, 2019

Same error :|

@ljharb
Copy link
Member

ljharb commented Jul 31, 2019

In that case it really does seem like we're doing something different for classes and SFCs, so that's a bug.

@monsieurnebo
Copy link
Author

Good to know!

@jstheoriginal
Copy link

@monsieurnebo can you confirm this is only happening inside of a nested function you’ve made that contains JSX and gets called inside of the return of the component? We had this happening in this scenario; but not if the prop is accessed directly in the return of the component.

Eslint seems to think the nested function is a SFC component itself so it thinks it’s missing props in the parameter of the nested function.

@monsieurnebo
Copy link
Author

monsieurnebo commented Aug 1, 2019

@jseminck I'm not sure to fully understand your scenario 🤔
Could you provide a simple example of what you're talking about?

@jstheoriginal
Copy link

jstheoriginal commented Aug 2, 2019

@jseminck I'm not sure to fully understand your scenario 🤔
Could you provide a simple example of what you're talking about?

Here's an example:

interface Props {
  data: {
    text: string,
    icon: Image,
  },
}

const Button = ({ data }: Props) => {
  ...

  const renderLabel = () => <Text>{data.text}</Text>;

  return (
    <View>
      {renderLabel()}
    </View>
  );
};

This ESLint rule should complain that data.text is missing in props validation. The reason seems to be because ESLint thinks that renderLabel const is a different component of its own and needs to have props of its own. The error goes away if you pass the props in to the parameters of renderLabel.

@monsieurnebo
Copy link
Author

monsieurnebo commented Aug 2, 2019

Thanks for the clarification, I have a better understanding of it now.
Yes, I confirm that's happening in a nested function such as:

import React from "react";
import sharedPropTypes from "./sharedPropTypes";

const propTypes = {
  ...sharedPropTypes,
};

function MyComponent(props) {
  const renderFoo = () => {
    <div>
      {props.steps.map(item => ...)}
    </div>
  }

  return (
    <div>
      {renderFoo()}
    </div>
  );
}

MyComponent.propTypes = propTypes;

And the error goes away by passing the props as you said:

import React from "react";
import sharedPropTypes from "./sharedPropTypes";

const propTypes = {
  ...sharedPropTypes,
};

function MyComponent(props) {
  const renderFoo = (passedProps) => {
    <div>
      {passedProps.steps.map(item => ...)}
    </div>
  }

  return (
    <div>
      {renderFoo(props.steps)}
    </div>
  );
}

MyComponent.propTypes = propTypes;

Passing props to nested functions can be a workaround for now, but it would be nice that ESLint understands it by itself (if that's something doable 🤔 ).

@saunders1989
Copy link

I was having the same issue on version 7.16.0 a work around for me was to downgrade to 7.13.0 hopefully this will be fixed

@jzabala
Copy link
Contributor

jzabala commented Jul 12, 2020

@ljharb I think we can close this one. As you said in #2357 (comment) importing propTypes won't work with static analysis and using a variable, defined in the same file, was fixed by #2704

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

No branches or pull requests

6 participants