Dead code eliminate React.PropTypes checkers in production

[gaearon]

I was looking if we can shave off some kilobytes here and there and noticed that we ship PropTypes code that we never actually run in production. I tried replacing it with a noop function with identical API and it seems to save 933 bytes off the gzipped react.min.js and 1035 bytes off the gzipped react-with-addons.min.js:

   raw     gz Compared to last run                                             
     =      = build/react-dom-server.js                                        
     =      = build/react-dom-server.min.js                                    
     =      = build/react-dom.js                                               
     =      = build/react-dom.min.js                                           
  +792   +130 build/react-with-addons.js                                       
 -3693  -1035 build/react-with-addons.min.js                                   
  +792   +138 build/react.js                                                   
 -3693   -933 build/react.min.js    

(The dev builds grew larger though, as we branch on __DEV__ now.)

As far as I can see this would only break tools that rely on introspection of PropTypes in the prod build but I don’t think if we ever supported this usage scenario anyway. Is there any other reason that I missed why this might be a bad idea?

[gaearon]

To clarify, I understand that we already don’t check propTypes in production. However we still bundle the PropTypes checker code itself. With this change, it is no longer referenced in the production code path, so Uglify dead code eliminates it. Nevertheless we keep the identical API (including stuff like PropTypes.oneOf(PropTypes.number).isRequired) so it does not crash, which is what would happen if we removed React.PropTypes in prod altogether.

[jimfb]

Seems reasonable. 👍

@facebook-github-bot shipit

[gaearon]

TODO before merging: check if new propTypes have been added. (e.g. #6377)

[ghost]

@gaearon updated the pull request.

[zpao]

I'm not convinced we should do this. It makes our API a lie and if we're going to do something like this, we should consider just removing PropTypes entirely in prod. I'm not convinced that's a good idea either though. In an ideal world I'd say we just let the code get removed by dead code removal in a packager but leave our API alone. Not sure that's a real option right now though.

cc @spicyj @sebmarkbage

[sophiebits]

Let's make it throw always? Maybe we could first warn too by passing a secret extra argument to every checker and warning if it isn't present (because that means you called the checker directly).

[zpao]

Let's make it throw always?

Make all proptype checkers throw? Would we try/catch that in dev?

[zpao]

Or I guess you're saying if the extra arg is there we return the error but if it's not we throw the error we would have returned?

[sophiebits]

As a temporary step: pass an extra fifth arg to every checker, warn when it's not passed (because that means people are calling PropTypes checkers manually and may break after the change).

Permanently: always throw if any PropTypes checker is called in prod. Then you at least notice if your code is calling PropTypes in prod.

[jimfb]

Warning seems less dangerous; less likley to change the flow of control in unexpected ways.

[sophiebits]

The point is to change the control flow if you're using it in an unexpected way. See Sebastian's XSS example.

[gaearon]

Let’s close and maybe revisit this some time later since there’s no clear conclusion.

[sebmarkbage]

@gaearon This is going to be the case for almost every issue. The key is to push through to a conclusion if you think it's important.

There's a plausible way forward here. @spicyj's proposal seems reasonable. We're not stalled and there are no pending dependencies so I'd suggest keep it open or an issue open so that a third party contributor can push it through.

[bloodyowl]

@gaearon wouldn't removing PropTypes break context, with contextTypes & childContextTypes being required to consume it?

[chicoxyzzy]

@bloodyowl it will. Anyway removing PropTypes is a breaking change so probably there will be another approaches to make context available in components

[gaearon]

@gaearon wouldn't removing PropTypes break context, with contextTypes & childContextTypes being required to consume it?

This PR doesn’t “remove” PropTypes—it makes them no-ops in terms of validation. So this PR doesn’t make any difference for context API.

[gaearon]

I filed #7131 as the first step to this.

[gaearon]

Resurrected in #7651.