Create-react-app: Removing propTypes in production build?

[timche]

What do you think about removing the propTypes in the production build to reduce the bundle size using babel-plugin-transform-react-remove-prop-types?

I've already prepared a fork, so if you think it is a good idea, I can submit a pull request.

[gaearon]

@spicyj Do you think this is reasonable for most apps?

[timche]

For reference:

• The advantage of this transform is saving bandwidth. We are not aware of any limitation.
• https://github.com/davezuko/react-redux-starter-kit/pull/561
• https://github.com/kriasoft/react-starter-kit/pull/440

[ForbesLindesay]

Seems like a good idea to me.

[insin]

Related: it would be cool if wrapping propType declarations in an if (process.env.NODE_ENV !== 'production') block became a common thing to make this automatic for your own code and particularly for your dependencies, for which you're probably using the transpiled ES5 they published to npm - is there a plugin for _that_?

Edit: Created an issue here: https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types/issues/35 - would be amazing to land this change and promote its use throughout the React ecosystem!

[vjeux]

We screwed up and didn't remove propTypes in dev when we started and now people rely on it existing in their app, so doing it now would be a big breaking change.

But for new apps that would be a good default imo. Now, it may be confusing that it is removed in this starter kit and not anywhere else

[gaearon]

Now, it may be confusing that it is removed in this starter kit and not anywhere else

I think it’s not a big deal because React will warn very soon when you attempt to call PropTypes validators manually: https://github.com/facebook/react/pull/7132. So people will know they’re not meant to be used in production, whether or not they use this kit.

[sophiebits]

This would probably be fine. I can imagine some people using it to mask the props object but hopefully not many. Can we add a linter against .propTypes?

[gaearon]

A linter against reading it?

[gaearon]

I think there are some valid use cases for accessing it, like

DangerButton.propTypes = {
  ...Button.propTypes,
  somethingElse: ...
}

[taion]

React-Bootstrap uses propTypes to mask props objects extensively, and a number of the components there would break outright if you stripped out propTypes in node_modules.

It's a fine optimization in most cases but it'd cause difficult-to-diagnose errors if you were to run it against node_modules.

[gaearon]

We wouldn’t touch node_modules, just the user code.

On the other hand there’s no reason why somebody shouldn’t be able to copy and paste a component from React Bootstrap and tweak it.

So I guess as long as people rely on this, we can’t do it. (Unless we specifically lint against it.)

[taion]

FWIW, this specific use case in React-Bootstrap is something like this: https://github.com/react-bootstrap/react-bootstrap/blob/v0.30.0/src/DropdownButton.js#L26-L27

We have parent components that render two child components, and we use propTypes to split out which prop goes where.

Looks hacky but it's fairly DRY and has worked well in practice.

[timche]

Thanks for the info! Looks indeed hacky, but if libraries rely on component propTypes, user code can rely on them as well and removing them can lead to unexpected behaviour in production. Seems like this can be closed then, right?

[taion]

I guess? It's a shame, though – that's a really cool transform, and I'm going to look at using it for production app builds now that I know about it.

[gaearon]

OK, I don't think the size reduction is worth the surprising behavior here.

[benwiley4000]

Quite glad you went with this decision - the performance benefit is likely relatively nonexistent and the size benefit seems questionable to me (if you really need to save space that badly, composing propTypes sounds like a bette idea to me). Knowledge of propTypes can be really useful if you're passing a very large number of props, some updating often, to a large number of components, each of which uses a small subset of those props. You can have your child component's shouldComponentUpdate screen for wanted props using propTypes and ignore the rest.

For my library I'm exposing an extendable class I'm calling PurePropTypesComponent (might need a better name):

class PurePropTypesComponent extends React.Component {
  shouldComponentUpdate (nextProps, nextState) {
    if (this.props) {
      // for props, only shallow compare keys found in propTypes
      for (const propName of Object.keys(this.constructor.propTypes || {})) {
        if (this.props[propName] !== nextProps[propName]) {
          return true;
        }
      }
    }
    if (this.state) {
      // for state, do normal shallow compare
      for (const key of Object.keys(this.state)) {
        if (!(key in nextState)) {
          return true;
        }
      }
      for (const key of Object.keys(nextState)) {
        if(!(this.state[key] === nextState[key])) {
          return true;
        }
      }
    }
    return false;
  }
}

If a user stripped propTypes out of a bundle the components would fail to update at all. The alternative - always updating when propTypes are missing - totally misses the performance benefit I was aiming for.

ETA: however reading this discussion convinces me a higher-order component might be better/more opt-outable implementation option for me.

[gaearon]

To be honest I feel like it might have been better to actually enforce stripping propTypes in user code. We might make a breaking change like this in the future.

We definitely don’t recommend relying on propTypes for any runtime behavior. It’s not at all obvious, and can lead to confusing times for anyone working on the project in the future.

[benwiley4000]

Aha - sorry I brought it up :P

Good to know though - not sure what's so bad about relying on propTypes, would love some elaboration. Also, not sure if there would now be a solution for my problem (wanting a reasonably automatic mechanism which could be leveraged by a developer to prevent too-frequent updates to components receiving many but consuming few props).

[gaearon]

not sure what's so bad about relying on propTypes, would love some elaboration.

It’s just absolutely not obvious that something that’s used for one purpose (DEV-only type validation) is also being used for some other purpose. People with experience of working on React projects would be confused if they bumped into a project where it mattered.

Also, not sure if there would now be a solution for my problem (wanting a reasonably automatic mechanism which could be leveraged by a developer to prevent too-frequent updates to components receiving many but consuming few props).

Why are they receiving props they aren’t consuming? If you want shouldComponentUpdate to bail out in these cases, why pass these props in the first place? Since any components below won’t be properly updated anyway.

[benwiley4000]

Im not building react router, but consider React Router, whose Route component passes a few props to every component it receives regardless of whether those props get used, because it's up to the user what the route component implementation looks like. In my case it's an audio player component which provides presentational control children with a large set of callbacks as well as some data props which update frequently (e.g. currentTime). There may be a better approach for this - this release is pre alpha - but I'm not sure what it would be if I want to provide users the flexibility to define their own UI.

[gaearon]

Im not building react router, but consider React Router, whose Route component passes a few props to every component it receives regardless of whether those props get used, because it's up to the user what the route component implementation looks like.

If that component doesn't care about those props and wants to short-circuit rendering even if some of them changes, it can render another component which is just a PureComponent.

function MyRoute(props) {
  return <MyPureRoute propICareAbout={props.propICareAbout} />
}

class MyPureRoute extends React.PureComponent {
  // ...
}

export default MyRoute

[benwiley4000]

Nice - this is a solution I hadn't considered in this instance.

However my fear is this kind of strips the ease of use I had associated with my original solution. It would be possible to implement a higher order component whose creator function takes a props array or hash - e.g. pureWithProps(props)(MyComponent) - but at this point I would be duplicating information already in propTypes unless I did the sensible thing, which would be passing in the component's propTypes. Yet then propTypes could be magically deleted at compile time!

I guess if propTypes need to be dev-only, they should be defined with something like Flow, not a first class object. IMO it's unreasonable to delete code written by a user just because you're assuming it only existed for consumption by React.

[gaearon]

My hunch is you should not be passing unused props in the first place, and you can use options to control that.

export default connectPlayer(MyComponent, {
  withCurrentTime: true
})

I don’t think it’s very valuable to provide every prop on opt-in basis. Just figure out common use cases, and implement options for those.

[benwiley4000]

Hm... again, this is close, while I see a few disadvantages:

1. In some cases MyComponent is actually ComponentFromLibrary and the desired props should be pretty tightly coupled to the component. We can do this at the module export level... which could be ok.
2. Might be weird or annoying for users to have to totally reproduce information that already exists in their propTypes. In many cases they'll probably ignore this guidance, and then be mad when their code breaks with React 17 or whatever. Hence my judgment that propTypes should be more annotations than code if they aren't to exist at runtime.
3. I really want to support an API that allows specification of a list of arbitrary components - adding a set of accepted props along with each would be an annoying requirement.

I think these are all workable - for point 3 I could just make it optional and pass all the props otherwise. It's an optimization users can opt-in to. For me that's probably fine.

But again - is an alternative syntax for propTypes under consideration? 😄

[gaearon]

In some cases MyComponent is actually ComponentFromLibrary and the desired props should be pretty tightly coupled to the component.

I don’t understand what it means 😞 .

It would help if you could show a complete realistic example of the API you want to achieve (not just the PureComponentWithPropTypes part but the whole audio player thing, and how you imagine it integrating with user defined components).

[benwiley4000]

Ok, sure - here's an example of what I've been working with. I subbed in an HOC for the component extension I shared earlier, but same idea.

import { AudioPlayer, PlayCtrl, ProgressCtrl, withPureProps, withAudioPlayer } from 'audio-player-lib';

// user-defined control
let Back30SecondsCtrl = ({ currentTime, onSetCurrentTime }) => (
  <div style={/* */} onClick={() => onSetCurrentTime(currentTime - 30)} />
);
Back30SecondsCtrl.propTypes = {
  currentTime: PropTypes.number,
  onSetCurrentTime: PropTypes.func
};
Back30SecondsCtrl = withPureProps(Back30SecondsCtrl);

ReactDOM.render(
  // we're assuming PlayCtrl and ProgressCtrl already only update for their prop types
  // thanks to the library doing that for us
  <AudioPlayer playlist={/* */} controls=[PlayCtrl, Back30SecondsCtrl, ProgressCtrl] />
  document.getElementById('somediv')
);

// or the context-driven API:

const PlayCtrlContainer = withAudioPlayer(PlayCtrl);
const Back30SecondsCtrlContainer = withAudioPlayer(Back30SecondsCtrl);
const ProgressCtrlContainer = withAudioPlayer(ProgressCtrl);

ReactDOM.render(
  <AudioPlayer playlist={/* */}>
    <div>
      <Back30SecondsCtrlContainer />
    </div>
    <div>
      <PlayCtrlContainer />
      <ProgressCtrlContainer />
    </div>
  </AudioPlayer>,
  document.getElementById('someotherdiv')
);

Note that in the first example the supplied components will be rendered as children. The possible solution I was discussing was to turn off the default prop types checking thing and just do this:

<AudioPlayer controls=[
  PlayCtrl,
  { component: Back30SecondsCtrl, props: ['currentTime', 'onSetCurrentTime'] },
  ProgressCtrl
] />

And in that case Back30SecondsCtrl would be given only the props it asks for, and the rest would get all of them. Or, I could just require prop specification.

Thanks for looking at this! 😄

[benwiley4000]

After considering it for a bit I've decided that (optional) explicit specification of a propNames array is actually a pretty good idea. Thanks for the suggestion!

[iamandrewluca]

Would this solve the problem?

const devProps = (component, props) =>
  process.env.NODE_ENV === 'development' && (component.propTypes = props)

devProps(MyComp, {
  someProp: PropTypes.string
})

[gaearon]

No because most dead-code elimination tools (including Uglify that we use) aren't smart enough to process this. Maybe Google Closure Compiler could do this but it’s too hard to integrate for CRA users.

So, does anyone still want to send a PR? I think we’d take it for 2.0.

[iansu]

Is this just a matter of applying babel-plugin-transform-react-remove-prop-types in the production webpack config?

[gaearon]

I think so, hopefully.

[gaearon]

(or rather in our Babel preset)

[iansu]

Ah, okay. It looks like that plugin also has a couple of options to consider. For example, do we want to strip PropTypes from packages in node_modules? I'm thinking no.

I'll put together a PR.

[gaearon]

Not from node_modules. (But also we don't run our preset on node_modules, only /dependencies version.)

[oliviertassinari]

Not from node_modules

Some library authors are using the "wrap" mode of babel-plugin-transform-react-remove-prop-types to get the propTypes removed from the bundles in production. This was causing confusion at the beginning. I haven't heard any complaints about it for months now.

[taion]

Yeah – discretion there should be left to library authors for now.

[gaearon]

The wrap mode would break for libs that enumerate propTypes to do filtering etc. I feel comfortable with asking people not to do this in their own apps, but I'm wary of breaking third-party code like this where there might not be an easy fix.

[probablyup]

Aren’t propTypes required for anything using context?

[gaearon]

No, contextTypes / childContextTypes are. Those aren't removed as far as I can tell.