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 #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?