React-router: Prevent PropTypes.js from showing up in production builds

[mbrevda]

It seems react-router/PropTypes.js is being includes in builds where NODE_ENV=='production'. Is there a need for this file in production?

[alisd23]

It's for devs who use propTypes on their components.

For example if you grab router off the context, you can use the exported propTypes

import { PropTypes } from 'react-router'

class App extends React.Component {
  //...
}
App.contextTypes = {
  router: PropTypes.routerContext
};

[mbrevda]

Perhaps I'm misunderstood, I was under the impression that proptypes are useless in production?

[alisd23]

That's true they're stripped in production but still - whilst in development, dev's might want the prop types, like in the example above

[mbrevda]

I'm wondering about production though - why is the file included in production bundles?

[alisd23]

Oh sorry I misunderstood, good point.

[alisd23]

I suppose it might be because if a dev does not strip propTypes in their bundle, any references to these propTypes will not exist, which could be unexpected behaviour.

It would basically add a _requirement_ for devs to have to strip propTypes from their code in production.

[mbrevda]

It would basically add a requirement for devs to have to strip propTypes from their code in production.

Considering this I'm not sure it's such a bad idea. Props are worthless in production anyway, encourage devs to strip them.

If a dev wants to keep the proptypes, they can manually include them. The proptypes should still be removed by default.

[alisd23]

I agree it should be encouraged.

Given this code:

import { PropTypes } from 'react-router'
class App extends React.Component {
  //...
}
App.contextTypes = {
  router: PropTypes.routerContext
};

This will be fine in _development_, however when the dev does a _production_ build PropTypes.routerContext would be undefined and would fail the build. So they would be forced to do this:

//...
if(__DEV__) {
  App.contextTypes = {
    router: PropTypes.routerContext
  };
}

That's what you mean right?

[mbrevda]

Or the inverse, where devs specifically include proptypes like: import propTypes from 'react-router'. I would encourage the first, but allow dev to fall back to the latter.

Note that making this change would add an almost 10% win to the final build size of RR!

[alisd23]

@ryanflorence @timdorr Thoughts?
Remove PropTypes.js from the production build and force devs to strip any propTypes which import these in their production build.
Or leave it as it is?

[mjackson]

We should always have a PropTypes.js file in our npm package. Our code relies on that file to run in development. That's not the problem.

The problem is that even when you strip propTypes from your components webpack isn't yet smart enough to realize you're not actually using that file and so it shouldn't include it in the final build. Maybe it would work if we put propTypes import statements inside the guard as well?

if (__DEV__) {
  import { PropTypes } from 'react'

  Router.propTypes = {
    // ...
  }
}

We currently use babel-plugin-dev-expression to substitute __DEV__ with false at build time, which essentially makes all code within the guard "dead code" so it can be eliminated. I'm just not 100% sure if that step runs before or after webpack has determined which modules it needs to import. If it runs before, it should work.

[timdorr]

imports have to be at the top level, so you'd have to use CJS for that.

[kwelch]

I would like to add that I really like the propTypes in the package. It has been extremely helpful to import it and use instead of hand declaring propTypes for context.

[mbrevda]

@kwelch I'm aware of the benefits of having propTypes during development. I'm advocating for production builds only.

[mjackson]

Well, I do think this is a worthwhile problem to solve. If you do solve it you'll be a hero for the whole React community. I'd imagine just about everyone has this problem.

[timdorr]

Add in this transform plugin, maybe?

[mbrevda]

That won't keep the file from being included @timdorr

[kennetpostigo]

Not sure if you guys are willing to make the change but I think rollup can treeshake those away. I think I saw @ryanflorence a couple days ago that he may use rollup to get some quick wins to shrink build size on twitter. Switching to rollup can help with that. In any case switching to rollup may not be ideal because I'm not sure exactly about how you guys orchestrate your build for react-router.

[mbrevda]

The way I see it, there are a few options here:

1. As @kennetpostigo suggested, switch to rollup or webpack v2 and start shaking em trees
2. Move proptypes into individual files - duplicate definitions notwithstanding - and use a the plugin @timdor recommended (or simply continue to wrap proptypes in __DEV__
3. Wrap proptypes.js's definitions in a ternary as below. Measured in bytes, this is least efficient option
4. Use conditional CJS imports for this specific file until tree shaking is more feasible

export const matchContext = if (process.env.NODE_END !== 'production') ? PropTypes.shape({
  addMatch: PropTypes.func.isRequired,
  removeMatch: PropTypes.func.isRequired
}) : {}

[AsaAyers]

Proptypes serve two purposes. SomeComponent.propTypes can be stripped in production, but SomeComponent.contextTypes can't. Even in a production build, I need access to the routerContext:

import { propTypes as rrPropTypes } from 'react-router'

const withRouter = (Component) =>  {
    const WrappedComponent = (props, context) => (
        <Component {...props} router={context.router}/>
    )
    WrappedComponent.contextTypes = {
        router: rrPropTypes.routerContext.isRequired
    }
    return WrappedComponent
}

Maybe PropTypes.js is just named wrong. Three of them are actually _context_ types and can't be removed in production builds.

[TheLarkInn]

FWIW: if you use webpack DefinePlugin you can strip out conditional expressions based on macro-like free vars.

new DefinePlugin({ENV: myNodeEnvVarObtainedInConfig})

The following would get stripped if in your code if the ENV was equal to "prod"

if ( ENV != 'prod') {
  const PropTypes = require('prop-types');
}

[mjackson]

@TheLarkInn Am I correct in assuming there is no way to do a conditional import (ES6 modules) in webpack? It must be a call to require?

[timdorr]

It's a spec requirement. imports (and exports) only exist at the top level of a module. Also, babel enforces it at the parsing level. No way around it.

[mbrevda]

I originally opened this issue with a question:

Is there a need for this file in production?

The answer it seems is yes, this file is required in production. While propTypes per se aren't used in production, this library makes extensive use of context, which are defined in the same way as propTypes. Hence these "propTypes" are in fact necessary during production. We will have to wait for better tree shaking and property mangling for greater gains.

The only thing we can do in the meantime is to rename some properties to be slightly shorter. See https://github.com/ReactTraining/react-router/pull/4083 for more.

[TheLarkInn]

Yes, that is correct the spec prevents it. This could change with import() but yes for now require is the only way