perf(props): Remove propTypes from production build

[layershifter]

For details - #524

This PR might to do:

• updates contributing guide;
• updates getUnhandledProps function;
• adds new sections for common tests.
• updates ComponentProps component in docs (we can't rely on meta anymore).

---

@levithomason I've updated Rail component, waiting for review there.

[jeffcarbs]

I think this could work, although I'm not too excited about if (process.env.NODE_ENV !== 'production') { everywhere. Especially when propTypes is defined as a static prop within the class.

What about this:

• define props within the meta as you're doing now. The newly added tests will ensure we're not missing any props.
• Instead of wrapping manually with NODE_ENV, use this as part of the build process: https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types

I'm still not 100% sure what the Rails.props are going to be used for, but I think if we made that a const it would get removed during dead code removal since it won't be referenced after the propTypes are removed:

const propValues = {
  close: ['very'],
  position: SUI.FLOATS,
  size: _.without(SUI.SIZES, 'medium'),
}

Rail.propTypes = {
  // snip ...

  /** A rail can be presented on the left or right side of a container. */
  position: PropTypes.oneOf(propValues.position).isRequired,
}

Or we could just inline them:

Rail.propTypes = {
  // snip ...

  /** A rail can be presented on the left or right side of a container. */
  position: PropTypes.oneOf(SUI.FLOATS).isRequired,
}

[levithomason]

babel-plugin-transform-react-remove-prop-types

I love this plugin, however, I don't think we can remove the propTypes in any of our builds since the user needs to have them in dev. If we strip them, then only we'll have them in dev. We need to only remove them when the user builds for production.

It is worth noting however that this plugin has a mode: 'wrap' option which does exactly what was done in this PR. It wraps propTypes in a NODE_ENV check. We should probably just use this!

NODE_ENV

I think it is acceptable to use NODE_ENV === 'production' since we're a React lib and React has force opted all users into this pattern. React itself uses these checks and includes instructions for it in their docs:

To use React in production mode, set the environment variable NODE_ENV to "production".

React router v4 is also wrapping propTypes in a dev check. So, I think it is here to stay for a while.

---

I've added the plugin with the mode: 'wrap' option and tested a build. Works perfectly 😄 :

Without Plugin

Flag.propTypes = {
  /** An element type to render as (string or function). */
  as: _lib.customPropTypes.as,

  /** Additional classes. */
  className: _react.PropTypes.string,

  /** Flag name, can use the two digit country code, the full name, or a common alias */
  name: _react.PropTypes.oneOf(Flag._meta.props.name).isRequired
};

With Plugin

process.env.NODE_ENV !== "production" ? Flag.propTypes = {
  /** An element type to render as (string or function). */
  as: _lib.customPropTypes.as,

  /** Additional classes. */
  className: _react.PropTypes.string,

  /** Flag name, can use the two digit country code, the full name, or a common alias */
  name: _react.PropTypes.oneOf(Flag._meta.props.name).isRequired
} : void 0;

I've also just found this plugin is included in the preset, babel-react-optimize. I'm going to try this out, if it works then I'll update this PR to use the plugin instead of manually adjusting the code. We'll still need this PR since we'll need the updates to our utils to not rely on propTypes.

[levithomason]

What's done

I've added some perf plugins:

babel-plugin-transform-react-constant-elements

Any const element in a render method, such as <label>foo</label>, is instead rendered once at the top of the file and replaced with a variable. This just prevents needless re-rendering of constant elements.

babel-runtime + babel-plugin-transform-runtime

Babel puts helpers in every file to allow future features (spread, class, etc). The babel-runtime is a module with these helpers as exports. The transform here simply references the module exports instead of re-writing every helper at the top of every file.

babel-plugin-transform-react-remove-prop-types

This plugin wraps our propTypes in a production check so that when users make a production build, the propTypes are removed.

What's left

Wrapping prop values

We still need to wrap prop values so they are removed in production. Otherwise we'll have massive lists of prop values left in our files because they are not inlined in propTypes, they are referenced.

We don't inline them because then we wouldn't be able to access the values. They would be buried in a propType function argument. We use the values to test component configurations in common tests. We also plan to use them for a component explorer in the future. So, I don't think we should inline them.

I'm also considering simplification here. We could drop the goals of auto testing valid prop value configurations and the component explorer in trade for inline prop values. That would save a lot of work. Feedback?

List of props

We still need a list of props in _meta so our utils can use it during runtime. getUnhandledProps must access a list of props so it knows which ones are "unhandled".

Given the amount of work that is there (updating and maintaining every file) I think it is worth considering our own babel-plugin to do this. It seems straight forward, read the keys of propTypes, and add them to a static array. We have quite a bit to gamble with on this given the number of hours required to do it by hand.

[layershifter]

List of props

Seems, babel plugin is good idea, I suggested it at the beginning of discussion, it can make code cleaner. I can start on it after Dimmer and Reveal merge.

What it must do?

Collect values of:

• Component.autoControlledProps (if its class and extends autocontrolledComponent);
• keys of Component.defaultProps;
• keys of Component.propTypes.

And ands it to Component._meta.props.

[jeffcarbs]

And adds it to Component._meta.props.

@layershifter - Adds it as Component._meta.props wrapped in NODE_ENV === 'production' so that it can be removed during dead code removal by a user's compiler but still available to them during dev.

[layershifter]

I started to work on babel-plugin.

[levithomason]

Great, can't wait to see this one come to life!

[layershifter]

@levithomason Hey, plugin was completed, take a look here. I've implemented cases for stateless and statefull component.

Seems, we need plugin's code to this PR. Where I can store plugin? I'm thinking about babel/plugins directory for code and test/babel/plugins for tests.

[levithomason]

Seems, we need plugin's code to this PR. Where I can store plugin? I'm thinking about babel/plugins directory for code and test/babel/plugins for tests.

Exactly what I was thinking 👍

EDIT

It would be nice to also include the README.md for it in the babel/plugins directory. The in/out snippets could be updated as well.

[levithomason]

I was just informed by @davezuko that we can use babel-plugin-lodash directly with our project if we structure it in a friendly format on publish. We just then need to set the id option:

lodash/babel-plugin-lodash#85

[jeffcarbs]

@layershifter @levithomason - thoughts on making this babel plugin slightly more generic and possibly either shipping it separately or at least making it usable outside this project? I've come across a few places in my app where I've wanted to use the pattern of consuming props and passing the rest through but don't want to rely on propTypes.

Instead of (SUI-react specific):

Component._meta = {
  props: ['children', 'className']
}

it could output (more general):

Component.handledProps = ['children', 'className']

Could name it babel-plugin-handled-props or something.

[layershifter]

@levithomason I've imported code to this PR.

• I'm having problem with fs module, there are no it in webpack's bundle because target: web, but I'll need to make assertation on pluginTests.js that requires file read of expected. Any ideas there?

• We need solution with wrapping prop values, I'll think that it will be simplier make them inline like in latest Rail.js in this PR:

close: PropTypes.oneOfType([
  PropTypes.bool,
  PropTypes.oneOf(['very']),
]),

[layershifter]

@levithomason Any response? 😢

[levithomason]

Whoops, just got done doing a maintenance sweep but overlooked this one :/ Sorry!

I try to hit all unread issues/PRs and anything labeled "Ready For Review" each time I do a maintenance sweep. I may not be available the rest of today but will get to this in the next sweep. I'll add the label so as not to miss it again.

[jeffcarbs]

I was just informed by @davezuko that we can use babel-plugin-lodash directly with our project if we structure it in a friendly format on publish. We just then need to set the id option:

I tried this in my project and wanted to report back. I analyzed my webpack-bundled files using https://github.com/th0r/webpack-bundle-analyzer (super cool tool, btw!). Here's the before/after (I think "parsed size" is post-minification, "stat size" is all the file sizes summed):

Before:

After:

Usage is just:

webpackConfig.module.loaders.push({
  test: /\.(js|jsx)$/,
  exclude: /node_modules/,
  loader: 'babel',
  query: {
    // ... snip
    plugins: [
      // ... snip
      'lodash', // Run module cherry-picker on lodash
      ['lodash', { 'id': 'semantic-ui-react' }] // Run module cherry-picker on semantic-ui-react
    ],
    // ... snip

Not bad for one line of config 😁 . Will be cool to see how much further this number drops once this PR makes it in 👍

[levithomason]

Awesome, I also have a branch in progress that generates alias files for all components, so we can:

import Button from 'semantic-ui-react/Button' 

@jcarbo I'm actually curios how that worked without that PR... Did you manually add index files for it?

babel-plugin-handled-props

I think this is a great idea.

[jeffcarbs]

@levithomason - I only added that one config line, no other code. The code in babel-plugin-lodash is a little hard to follow, but I'm pretty sure it uses id as the root and will try to file modules loaded anywhere underneath. So given:

{ 'id': 'semantic-ui-react' }

I think it will do the following conversion:

// from
import { Button } from 'semantic-ui-react'

// to
import Button from 'semantic-ui-react/dist/commonjs/elements/Button'`

[layershifter]

Result of lodash plugin looks awesome 👍

---

@levithomason So what we do with plugin?

After @jcarbo offer I think we need to do this:

• leave plugin in separate repository and name it babel-plugin-transform-react-handled-props (I'll add you to collaborators);
• plugin will move array of props to Component.handledProps;
• we choose simple way and make prop values inline:

close: PropTypes.oneOfType([
  PropTypes.bool,
  PropTypes.oneOf(['very']),
]),

[jeffcarbs]

@layershifter - that sounds good to me! I think inlining the options is the cleanest way to do it. In most cases, those will be constants so I think it will actually read pretty clearly (e.g. PropTypes.oneOf(SUI.COLORS))

@levithomason - thoughts?

[levithomason]

I agree on all counts here. Let's do it.

[tomitrescak]

@davezuko what needs to be emitted there? I am emitting 'development' if I'm in development build, otherwise I leave it empty.

[EDIT] I am actually emitting 'production'

new webpack.DefinePlugin({
      'process.env': {
        NODE_ENV: 'production'
      }
    })

[EDIT2]

The production veriable is specified correctly and code removal is ok, as I am eliminating in the same way mobx-react-devtools. This is the size I am seeing now. It is the same as during development build:

[dvdzkwsk]

@tomitrescak that's exactly what I meant, and it was just a guess at that, so perhaps I'm on the wrong track. I just casually saw this issue and figured I'd inject a thought, but I'll keep thinking on it.

As an aside, I'm curious about that snippet you posted. I'm fairly certain that, at least at some point, that was not valid code for the define plugin due to how it injected globals; you'd actually have to stringify production (i.e. JSON.stringify('production')) so that the value of NODE_ENV was "production" (includes the quotes). This was because webpack inserted the value as is, so it would ultimately look like:

// without stringifying (NODE_ENV: 'production')
if (process.env.NODE_ENV === 'production') // before compile
if (production === 'production') // after compile, ERROR! production is not defined

// vs (NODE_ENV: JSON.stringify('production') [or just '"production"']).
if (process.env.NODE_ENV === 'production') // before compile
if ("production" === 'production') // after compile, no error

This is a whole 'nother tangent, and you'd likely have already encountered this problem in previous builds. I could be totally off base here, but it's all I can think of at the moment.

Aside #2:

Does webpack inject this global into non-webpacked packages? Meaning, you import SUIR, but webpack simply imports it, but does not transform it in any way (i.e. with babel and the lot), so perhaps it is not stripping out that code? Yes, process.env.NODE_ENV will be available and defined at runtime, but my gut tells me it is not applying that global statically into the SUIR source. I have honestly not ventured too far down this path (meaning optimizing external libs) so I'm really just spitballing... will do some research. This is most likely the issue.

[tomitrescak]

@davezuko that's exactly how I had it with JSON.stringify, I just changed it for this example.

I think it's just BundleAnalyser acting up. When I go the minified source code of semantic-ui-react the PropTypes are missing (in dev build they are there), yet BundleAnalyser reports exactly the same size.

[EDIT] An it's most probably true:

If you minify/compress your code using the approach recommended in the Webpack documentation, which is to use the UglifyJS plugin, be aware that this tool will report the sizes of modules before they are minified. This is because the statistics generated by webpack --json do not take account of plugins that operate on the bundle as a whole.

[layershifter]

Does this PR wraps propTypes and removes them from production build?

Yep, it makes this.

Why bundle is still have nearly same size?

We used _meta on components for tests and docs. Let me show it on Flag:

const names = [] // long array

function Flag (props) {}
Flag._meta = {
  // something there
  props: {
    name: names
  }
}

Yes, propTypes will be removed in production build, but _meta still require this long array of names, so it still be in bundle 😢

Guys, make something with it!

Yep, I'm working on it, for example, Flag was modified in #1155. But there ~30 components that need to be updated (#524).

Before Flag update:

 semantic-ui-react: 965.91 KB (59.5%)
   lodash: 291.54 KB (30.2%)
   <self>: 674.37 KB (69.8%)

After:

semantic-ui-react: 961 KB (58.9%)
  lodash: 291.54 KB (30.3%)
  <self>: 669.46 KB (69.7%)

Any ETA?

As soon as I can. Lowering bundle size is my priority in this time.
After all components will be updated, bundle will lose SUI.js and some other definitions.

[levithomason]

bundle will lose SUI.js

We'll likely lose some definitions in this file and it will become drastically smaller since things like ICONS_AND_ALIASES will move into propTypes and doc utils (for the icon search in the docs). However, the file will likely remain since we make heavy use of the first ~30 lines in almost every component.