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

prop-types for stateless function components #237

Closed
SimenB opened this issue Oct 7, 2015 · 30 comments
Closed

prop-types for stateless function components #237

SimenB opened this issue Oct 7, 2015 · 30 comments

Comments

@SimenB
Copy link
Contributor

SimenB commented Oct 7, 2015

prop-types doesn't seem to trigger when using https://facebook.github.io/react/blog/2015/09/10/react-v0.14-rc1.html#stateless-function-components

@jimbolla
Copy link

jimbolla commented Oct 7, 2015

How could it? Stateless function components don't have a way of defining proptypes. It would be impossible for one to satisfy the rule.

@SimenB
Copy link
Contributor Author

SimenB commented Oct 7, 2015

The same way as adding to classes without using static

const Detaljer = ({ detaljer, feilkode }) => {
  if (Array.isArray(detaljer)) {
    return (
      <ul>
        {detaljer.map((detalj, i) => <li key={i}><Detalj detalj={detalj}/></li>)}
      </ul>
    );
  }

  return (
    <div className="enkeltmelding">
      <Detalj detalj={detaljer}/>
      {feilkode ? <span className="kursiv-skrift">Feilkode: {feilkode}</span> : null}
    </div>
  );
};

Detaljer.propTypes = {
  detaljer: PropTypes.string.isRequired,
  feilkode: PropTypes.string,
  removeEvent: PropTypes.string.isRequired
};

image

@yannickcr
Copy link
Member

I did not know that stateless functions could have some propTypes like a regular component .

It is not supported by the plugin yet but I will work on it.

@jimbolla
Copy link

jimbolla commented Oct 7, 2015

Hmmm. Neither did I. Good to know!

@SimenB
Copy link
Contributor Author

SimenB commented Oct 7, 2015

It's in the release notes as well 😄

http://facebook.github.io/react/blog/2015/10/07/react-v0.14.html#stateless-functional-components

Functional components do not have lifecycle methods, but you can set .propTypes and .defaultProps as properties on the function.

@jimbolla
Copy link

jimbolla commented Oct 7, 2015

Yeah I just saw that. I don't believe that was mentioned in the blogpost for 0.14 RC1. Now I'm going back and re-adding propTypes to my converted components.

@trevordmiller
Copy link

I ran into this today; here is an example:

pure-function-components

@trevordmiller
Copy link

Thank you for all your great work @yannickcr! Your plugin has been very useful :)

@dbrans
Copy link

dbrans commented Oct 16, 2015

I don't think eslint will ever be able to know the difference between a function that returns jsx and a React "stateless component", without a hint like a propTypes member.

@SimenB
Copy link
Contributor Author

SimenB commented Oct 18, 2015

Ah, sweet! Can't wait for the release 😄

@yannickcr
Copy link
Member

I hope it will works fine, like @dbrans said, it is hard to detect is we are in a stateless component. So I had to do some compromises (the function need to return some JSX or a call to React.createElement).

I will document it in the rule documentation.

@gajus
Copy link

gajus commented Nov 30, 2015

the function need to return some JSX or a call to React.createElement

@yannickcr another method would be to check if the first parameter is props, as that would be almost always the case by convention.

(props) => {};

@kumarharsh
Copy link

@gajus - what if someone does this:

({name, id}) => {};

@kumarharsh
Copy link

FWIW, I've used this function in one of my codemods:

const isPossiblyPureComponent = path => {
  if (path.parent.node.type === 'CallExpression')
    return isPossiblyPureComponent(path.parent);
  else if (path.parent.node.type === 'VariableDeclarator')
    return true;
  return false;
};

j(file.source)
    .find(j.ArrowFunctionExpression)
    .filter(
      p => isPossiblyPureComponent(p)
          && (
            p.node.body.type === 'JSXElement'
            || j(p.node.body)
                .find(j.ReturnStatement)
                .find(j.JSXElement)
                .nodes()
                .length
          )
    )

which did the trick of detecting Pure components pretty well, although it's I think too heavy

@davidtheclark
Copy link

@yannickcr: Looks like you have the same conundrum as oliviertassinari/babel-plugin-transform-react-remove-prop-types#1 (cc/ @oliviertassinari), which is that it's difficult to detect whether a function is in fact a stateless component. I see an elaborate method in https://github.com/reactjs/react-docgen/blob/713e848b80bc7e93778c4c6666d1cfde4d8e6e29/src/utils/isStatelessComponent.js, and I wonder if @fkling has any interest in making that a module of its own so other tools (like eslint-plugin-react and babel-plugin-transform-react-remove-prop-types) can use it. Any thoughts?

@prithsharma
Copy link

prithsharma commented Jan 19, 2017

@yannickcr #237 (comment)
Regarding the check of should return some JSX -
I think I might have hit a minor bypass to the implementation of this check. The check works well when there is a return <some_jsx /> and thus the prop-types validation is run.

But if the function does something like below -

const returnValue = <some_jsx />;
....
return returnValue;

then it seems that the plugin doesn't recognize it as returning some JSX and thus the prop-types validation is not run.

@ljharb
Copy link
Member

ljharb commented Jan 20, 2017

@prithsharma please file a new issue for that, and we can take a look

@prithsharma
Copy link

Here you go - #1046

@camloken
Copy link

camloken commented Mar 27, 2017

If you have no logic in your component you can even make it a bit more concise:

const MyComponent = (props) => (
   <div>
      <b>{props.firstname}</b>
   </div>
);

MyComponent.propTypes = {
   firstname: React.propTypes.string,
};

export default MyComponent;

Notice there is no return statement or curly brackets on the component.

@ljharb
Copy link
Member

ljharb commented Mar 27, 2017

(although then your component doesn't have an explicit name; it's best to use normal functions, not arrows, for SFCs)

@kumarharsh
Copy link

@ljharb - If I'm not mistaken, theoretically you can do:

MyComponent.displayName = "MyComponent";

to retain the component name. Although it'll be writing characters to save a few characters ;)

@ljharb
Copy link
Member

ljharb commented Mar 28, 2017

Exactly right; but I'm also talking about the function's name property, which is useful for debugging, and more reliable when explicit.

@armenzg
Copy link

armenzg commented Sep 25, 2017

If it helps someone, I was having trouble with this ('post' is missing in props validation (react/prop-types)):

export default Post = ({ post }) => (
  <div>{post.title}</div>
);

Post.propTypes = {
  post: PropTypes.arrayOf(PropTypes.object).isRequired,
};

but this did not have anymore issues:

const Post = ({ post }) => (
  <div>{post.title}</div>
);

Post.propTypes = {
  post: PropTypes.arrayOf(PropTypes.object).isRequired,
};

export default Post;

@ljharb
Copy link
Member

ljharb commented Sep 25, 2017

@armenzg that seems like a bug. Can you file that as a new issue?

@armenzg
Copy link

armenzg commented Sep 25, 2017

@ljharb I've filed it:
#1447

Thanks!

@amackintosh
Copy link

amackintosh commented Nov 11, 2017

I just had this issue and it turned out to be a typo that wasn't highlighted by ES Lint.

Problem:

import React from 'react'
import PropTypes from 'prop-types'
import { NavLink } from 'react-router-dom'
import { Wrapper, Button, Menu, MenuItem } from 'react-aria-menubutton'

// these two props were being highlighted as missing proptypes (which was actually true)
const ListActionsMenu = ({ thingRoute, serialNumber }) => {
  const MENU_ITEMS = [
    <li key='view'>
      <MenuItem>
        <NavLink to={`/${thingRoute}/view/${serialNumber}`}>View</NavLink>
      </MenuItem>
    </li>,
    <li key='edit'>
      <MenuItem>
        <NavLink to={`/${thingRoute}/edit/${serialNumber}`}>Edit</NavLink>
      </MenuItem>
    </li>,
    <li key='deactivate'>
      <MenuItem>
        <NavLink to={`/${thingRoute}/deactivate/${serialNumber}`}>Deactivate</NavLink>
      </MenuItem>
    </li>
  ]

  return (
    <Wrapper
      className='list_results-actions'
      onSelection={() => null}>
      <Button className='list_results-actions-icon'>
        <i className='fa fa-more-horiz' />
      </Button>
      <Menu className='list_results-actions-menu'>
        <ul>{MENU_ITEMS}</ul>
      </Menu>
    </Wrapper>
  )
}

// but, it was due to this capitalized P
ListActionsMenu.PropTypes = {
  thingRoute: PropTypes.string.isRequired,
  serialNumber: PropTypes.string.isRequired
}

export { ListActionsMenu }

@yannickcr @ljharb I figured I would just drop this example scenario in here, in case it is desired to analyze for any API hardening.

I was really confused why it was saying there was no prop types, and that would have been a 2 second fix if ES Lint had warned me that PropTypes needed a lower case p. I can see how it could be tricky to add a case for such anomalies though.

It could be nice to somehow get a red underline on Something.PropTypes = { if there is a capital P.

@ljharb
Copy link
Member

ljharb commented Nov 11, 2017

There’s already a no-typos rule that should handle that.

@joelaguero
Copy link

joelaguero commented Nov 12, 2017

I also just ran into @amackintosh issue. I was able to trigger the typo detection by including react/no-typos in my .eslintrc.js file.

"rules": {
  "react/no-typos": 1,
},

@ViggoV
Copy link

ViggoV commented Jan 8, 2019

This seem to still be an issue. For stateful components missing prop-types are detected, but for a stateless functional component I get zero errors. eslint 5.12.0 and eslint-plugin-react 7.12.3.
I believe it has worked before, probably with an older React version, but I am not entirely sure. I was wondering if it could have something to do with multiple HOC wrappers, but both the functional and stateful components I am looking at are using more or less the same set of HOCs.

@ljharb
Copy link
Member

ljharb commented Jan 8, 2019

@ViggoV if so, please file a new issue with as much code and eslint output as possible.

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

No branches or pull requests