Ignored static properties of SFCs (Stateless Functional Components)

[mbrowne]

This library respects the use of defaultProps on SFCs, which is good (although in practice you can often just use default argument values for SFCs), but there are other properties an SFC can have that are currently ignored:

• propTypes
• contextTypes
• displayName

I realize that calling a function directly is more efficient than mounting it as a component [1], so skipping displayName makes sense, but IMO propTypes and contextTypes should still be respected, if they exist. So in those cases I would think it would be better to go ahead and render the SFC as a component rather than calling it directly.

Example of PropTypes being ignored:

function Test({ render }) {
    // this should trigger a warning since we're passsing a number and not a string
    return renderProps(render, { name: 123 })
}

function Greeting({ name }) {
    return <div>Hi, {name}</div>
}

Greeting.propTypes = {
    name: PropTypes.string.isRequired
}

export default class App extends Component {
    render() {
        return (
            <div>
                <Test render={Greeting} />
            </div>
        )
    }
}

[1] FWIW, SFCs have new optimizations applied to them in React 16 that make them faster than regular class components ("regular class" meaning React.Component without a shouldComponentUpdate method...as to SFC vs. React.PureComponent, it depends on the situation).

[mbrowne]

P.S. I suppose contextTypes might become irrelevant in React 16.3...

[donavon]

@mbrowne,

Very good points. Calling a SFC directly (as I'm doing) indeed does bypass PropTypes validation, which it shouldn't. I've been pondering this and have come up with the following.

If the code is running in development, call as a component. This would give full props validation. If running in production, call directly as props validation isn't done in production anyway. Best of both worlds(?)

I know that this doesn't answer your contextTypes concern. One thing at a time. 😉

Thoughts?

p.s. I'm also thinking that you could control this behavior with a static 'Component.options` object, but I'm not as set with this just yet.

[mbrowne]

I think your proposed solution is logical, I just wonder if it might ever cause issues that the production behavior would be different than in development...perhaps there are some edge cases where code could depend on the component being mounted instead of just called as a function. This seems unlikely but I would be a little concerned with this mainly because it would happen behind the scenes, and probably not be apparent to the programmer. If you still want to provide the performance optimization in the case of SFCs with propTypes, it looks like you could take an approach similar to what you did with defaultProps. I took a look at how React checks prop types and I see that it uses the checkPropTypes, which is a public function of the prop-types package:

import checkPropTypes from 'prop-types/checkPropTypes'

Here's one of the places where they call it:
https://github.com/facebook/react/blob/master/packages/react/src/ReactElementValidator.js#L235

The only issue I can think of with calling it directly rather than letting React handle it would be the stack traces. But I'm guessing that's a solvable problem, and in any case the getStack parameter is optional:
https://github.com/facebook/prop-types/blob/master/checkPropTypes.js#L25

Your proposed solution might be fine, but I think it's worth considering this alternative.

Thanks!

[donavon]

I implemented my solution for now (published on npm as v1.1.0), but could always be open to circling back if there is a stong case to do so.

I'll go ahead and close this for now. Thx for your thorough consideration of the problem and for raising the issue.

fixed with #4

Also: see https://codesandbox.io/s/6j2k7z35nn too see prop types warning

[mbrowne]

Thanks! And as to contextTypes, I suppose it makes sense to wait and see whether or not those are deprecated in the production release of React 16.3.

[donavon]

You can open a separate issue for just contextTypes if you wish (linking it to this one) so we can track it.

[mbrowne]

Oh, I just noticed that the way you implemented it (the handling of propTypes) doesn't do anything differently in production than in development, which you said you were considering above. So yeah, I don't see any potential issues with this implementation. Just thought I'd clarify, mainly for anyone who reads this thread in the future.

[donavon]

@mbrowne I need to clarify your clarification...

My approach MAY result in different behavior in development vs. production IF you use a plugin for dead code/propType elimination based on process.env.NODE_ENV (which many do).

If static propTypes is there in development my code will render your SFC as a component. Later, when you do a production build, and the plugin removes the static propTypes, my code would call your SFC directly as a function.

I see this as a good thing, but if you have problems because of this, file an issue with your specific use case.

[mbrowne]

Ah ok, I understand it better now, thank you.