Eslint-plugin-react: Allow prop-types rule to skip non-exported SFCs

[grrowl]

From the docs:

For now we should detect components created with:

• React.createClass()
• an ES6 class that inherit from React.Component or Component
• a stateless function that return JSX or the result of a React.createElement call.

It would be great if we could enforce propTypes being defined on React.createClass() and React.Component-extended components, but not stateless functions which return JSX.

In fact, defining .propTypes in function use such as { arr.map((name, i) => <Row key={ i }>{ name }</Row>) } would increase verbosity considerably

[ljharb]

array method callbacks may make sense to skip, but standard SFCs should definitely always have propTypes.

[darrennolan]

I'm having a weird issue at the moment with prop-types.

import VideoJSPlayer from '../../components/videojs-player';

export default function getVideoJSComponentStream({
  videoJsControlBus
}) {
  // do lots of fancy stuff
  return <VideoJSPlayer videoJsControlBus={videoJsControlBus} />
}

I end up with warnings for videoJsControlBus missing in props validation.
But VideoJSPlayer is a component defined elsewhere, imported in at the top - that component definition has the correct prop types.

I _think_ it's detecting this particular case as a stateless function component, which technically it's not?

Thus ignoring stateless components would be a highly desirable option.

[ljharb]

@darrennolan in this case, getVideoJSComponentStream should indeed have propTypes - however you could do getVideoJSComponentStream.propTypes = { videoJsControlBus: VideoJSPlayer.propTypes. videoJsControlBus };.

The goal is to catch bugs as soon as possible - even though VideoJSPlayer will catch it, _your_ component should be catching it first.

[darrennolan]

Thanks for the super quick reply @ljharb =D

But getComponentStream doesn't actually return a new react component. I should update that code a bit better.
It's returning an observable stream, just happens to map to a react component, that's typically injected into another component.

To me, that would be like having to declare prop types against the following, when Class PlayerComponent already has them defined.

ReactDOM.render(
    <PlayerComponent {...data.view} />,
    this.element
);

Unless in these cases it's better for me to explicitly return React.createElement(SomeComponent, someProps) ? Which, maybe it would be better?

[ljharb]

Could you provide a more accurate example? If your function doesn't directly return JSX, it shouldn't be identified as needing propTypes.

[darrennolan]

Trimmed down version of the function, but this shows the streams we muck about with and then return a stream that when subscribed - gives us this react component with the props set accordingly.

import React from 'react';
import bacon from 'baconjs';
import isEqual from 'lodash/isEqual';
import get from 'lodash/get';

import VideoJSPlayer from '../../components/videojs-player';

export default function getVideoJSComponentStream({
    autoPlayStream,
    marbleStream,
    marbleVideoSourcesStream,
    preloadStream,
    showAdStream,
    videoJsControlBus,
    videoJsEventBus
}) {
    let videoJsSettings = bacon.combineWith(
        marbleStream, marbleVideoSourcesStream, autoPlayStream, preloadStream,
        (marbleVideo, marbleVideoSources, autoplay, preload) => {
            return {
                autoplay,
                preload,
                marbleVideoSources: marbleVideoSources,
                nativeControlsForTouch: true,
                poster: get(marbleVideo, 'posters.large', '')
            };
        }
    );

    return bacon.combineWith(
        videoJsSettings, showAdStream, marbleStream,
        (videoJsSettings, showAd, marbleData) => ({videoJsSettings, showAd, marbleData})
    )
        .skipDuplicates(isEqual)
        .map((settings) => {
            return (
                <VideoJSPlayer
                    showAd={settings.showAd}
                    videoUid={get(settings, 'marbleData.marbleId')}
                    videoJsControlBus={videoJsControlBus}
                    videoJsEventBus={videoJsEventBus}
                    videoJsSettings={settings.videoJsSettings} />
            );
        });
}

The props needed warning appears on the function getVideoJSComponentStream arguments. Which may or may not be expected.

[grrowl]

I'm not sure what the core objection is here — we use PropTypes extensively across our application, at multiple boundaries and depths. We only use Stateless Function Components within class components with propTypes defined (and enforced, thanks to this plugin) so it's both unnecessary to re-declare the same propTypes again for SFCs and affects readability of our code.

I understand not everyone works in this way, which is why it would only be optional at best. I don't have experience with ASTs and eslint but if it's too hard to deal with here I'll try to find some time soon to get a PR together.

Thanks.

[ljharb]

@darrennolan that looks like a bug in that it's mistaking the JSX in the .map for your function being an SFC. i'd file a separate issue about that.

[darrennolan]

Thanks @ljharb - I've made a new ticket at https://github.com/yannickcr/eslint-plugin-react/issues/504 (hoping I copied all the relevant information).

[ianstormtaylor]

+1, turning this off for stateless function components is exactly what I need. They get used for one-offs, which don't make sense to turn on prop type checking for.

[ljharb]

I would be fine with a rule stating that un-exported SFCs don't need propTypes.

However, any exported SFCs should have the same propType checking as any other kind of exported component, and I would be a strong -1 on any propTypes rule that differentiated between components solely based on their statefulness/statelessness.

[ianstormtaylor]

That happens to work for my use case, so fine by me.

Although I have to say, I don't really understand the extremely rigid take on it. The whole point is that it's an option you wouldn't have to ever use if you disagree so strongly with it. But for other folks it makes sense that when they write one-or-few-liner components they don't want to have to be forced to add prop types.

Either way, I've just abandoned this rule now anyways.

[paulyoung]

I would be fine with a rule stating that un-exported SFCs don't need propTypes

This would be great. Is there any progress towards this yet?

[amannn]

I also get a false negative for this scenario:

The rule proposed in this thread would help for my use case.

[ljharb]

It's not a false negative, it's a correct error - but yes indeed, this proposal would stop warning there.

[amannn]

Really? Even though that function is called directly and not with React.createElement?

[ljharb]

@amannn ah sorry, i misread. yeah, in that case it's our SFC detection, since the function returns jsx and isn't a callback, it looks like an SFC.

[bdwain]

if i have

function foo(props){
   return <div>{props.whatever2}</div>;
}

function ActualComponent(props){
  return (
   <div>
      <div>{props.whatever}</div>
       {foo(props)}
   </div>;
}

ActualComponent.propTypes = {
   whatever: PropTypes.string.isRequired
};

export default ActualComponent.

the linter should recognize that foo is not a component and doesn't need proptypes, and that ActualComponent is actually using the whatever prop, and that acutal component is missing a proptype for whatever 2, right?

is it possible to trace through the function calls to see which props are getting used by helper functions? and to not expect proptypes on functions that are called explicitly by other parts of the file?

[ljharb]

The recommendation there is that foo should in fact be an SFC, that does need propTypes.

That said, since foo has a lowercase name, the component detection should definitely be able to not treat it as a component - but at that point, whatever2 wouldn't be traceable backwards - which is part of the reason that you don't want to have a function that takes props and returns JSX that isn't actually a component.

[bdwain]

that makes sense.