[PropTypes] Name PropTypes imports

[stephenburgess8]

• PR has tests / docs demo, and is linted.
• Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
• Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Would using named imports affect speed? Is there a compelling stylistic or architectural reason for using a named import of import {PropTypes} from 'react' or the alternative of repeatedly calling React.PropTypes.{...}?

For instance with a named import of import {PropType} from 'react', the PropTypes declaration would look like

propTypes: {
  location: PropTypes.object.isRequired,
  onRequestChangeList: PropTypes.func.isRequired,
  open: PropTypes.bool.isRequired,
}

and with a default export of import React from 'react' it would look like

propTypes: {
  location: React.PropTypes.object.isRequired,
  onRequestChangeList: React.PropTypes.func.isRequired,
  open: React.PropTypes.bool.isRequired,
}

In this excellent question from 2013 on import module vs. from module import function in Python, the answers seem to center mostly on stylistic and architectural reasons for choosing between default and named imports. Is this true in React as well or is there a difference in performance?

This PR reduces the size of the MUI source code by 7335 bytes using the named import over about 2,300 lines.

Make code cleaner by eliminating clutter of calling PropTypes from React default import. Use named import of {PropTypes}. Rename MUI custom PropTypes to MuiPropTypes to avoid conflict. Fix linting errors in src/svg-icons/index.js.

UPDATE: Both versions render in the same amount of time when averaged (2.4s). The new rendered version drops the package from 6.9mb to 6.8mb, with a difference in the size of app.js of 21.39kb.

[stephenburgess8]

Error text:

[execsync v1.0.2] Attempting to compile native extensions.

No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

Looks like it timed out, no reason why is immediately obvious. I looked over the diff again to see if there were any typos but wasn't able to find any.

On the Requests page, it's saying build 4412 was successful:

While on the actual build page it says it errored. https://travis-ci.org/callemall/material-ui/builds/116216257

I will look for the typo that may still be passing all the unit and lint tests but is failing the Travis build. Is it possible to try to run this again?

I am curious to see if this PR would affect the speed of the library in any way. It reduces size by about [7.3kb]. On one hand, that's not much next to the total size of the library. On the other, there are some micro libraries out there that aren't much bigger than 10kb. The Redux src is 18kb.

One test I could run is going through the docs instance and selecting every component. If I can't find a typo, I will try that.

EDIT: reduces size by 7.3kb.

[nathanmarks]

@stephenburgess8 Nothing to do with your code, travis crapped out.

[mbrookes]

Review aside, on hold pending #3682.

Even though there's more to be done there, there's too much work in that PR to have this one merged first and create a ton of merge conflicts, when this one is just a simple global search and replace.

[alitaheri]

@stephenburgess8 I understand that your work has 2 steps: replace the current PropTypes with MuiPropTypes and then React.PropTypes with PropTypes. It would be best if you could break this down into 2 PR. This way it will be a lot easier to review and a lot easier to merge without conflicts.

[oliviertassinari]

I am curious to see if this PR would affect the speed of the library in any way. It reduces size by about [7.3kb].

If you are using a tool like https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types/ in production,
that may be doing more harm than it's helps. I'm not 100% sure. I would need to check if uglify can remove or not the extra PropTypes import.

[oliviertassinari]

Actually, the babel output is almost the same. I'm fine with this changes 👍.

in

import React, {PropTypes} from 'react';

const contextTypes = {
    router: PropTypes.object,
    pushProfilesRoute: React.PropTypes.func
};

out

'use strict';

var _extends = Object.assign || function (target) { for (var i = 1; i < arguments.length; i++) { var source = arguments[i]; for (var key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } } return target; };

var _react = require('react');

var _react2 = _interopRequireDefault(_react);

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

var contextTypes = _extends({}, Component.contextTypes, {
     router: _react.PropTypes.object,
     pushProfilesRoute: _react2.default.PropTypes.func
});

[mbrookes]

This way it will be a lot easier to review and a lot easier to merge without conflicts.

#3682 touches imports on every component. There will be massive merge conflicts if this gets merged first, whether together or split. This is a novelty PR; let's park it and focus on the real work.

[stephenburgess8]

This PR is certainly of the lowest priority, though I wouldn't consider the attempt to reduce the size of a 7mb library to be novelty. I'll watch #3682 to see when it's done, then split this into two.

[newoga]

I think this a good idea. I haven't looked at the code changes, but I faintly remember there being a few cases where a PropTypes variable was used that pointed to an internal module. I'll look into that more closely later...

Edit: Nevermind, looked in master, at some point we changed all the PropTypes to propTypes so this should be safe.

[stephenburgess8]

@newoga There is an internal PropTypes that I renamed mui-prop-types that serves to limit custom props to one of a few options, such as positioning and z-depth. This serves a slightly different function than React proptypes which serve to say what kind of variable it must be and doesn't enforce only certain options of that variable type. As such it would be good to differentiate between the two. You can mui-prop-types here . I didn't change the content, just the name. Thanks.

[newoga]

@stephenburgess8 Thanks for the follow up. It's probably safe to continue this effort since we got the ES6 classes PR merged.

Also @stephenburgess8, is there any benefit to using class MyComponent extends Component instead of class MyComponent extends React.Component?

[stephenburgess8]

@newoga I will try this again with the refactored code.

The major benefit of the change proposed in this PR is to reduce bytes as a result of character boilerplate. Doing a named import of component would likely result in a slight increase of characters due to a one-to-one relationship between referencing React.Component and components. For each component, React.Component is called once. On the other hand, for every component, React.PropTypes is called several times on average. That's where the savings in bytes would be coming from.

I'll get back to work on this. Thanks.

[oliviertassinari]

The major benefit of the change proposed in this PR is to reduce bytes as a result of character boilerplate.

As far as I have tested it, once Babel transpile the code, that doesn't make any difference in production.
I believe that the advantage is conceptual. You import what you need from packages.

[nathanmarks]

@oliviertassinari @stephenburgess8 also -- don't spend too much time on this as I can automate it pretty easily.

[nathanmarks]

@oliviertassinari especially using the transform 👍

[stephenburgess8]

In my testing (previous version) it reduced the size of the rendered app.js by 21.39kb.

[oliviertassinari]

In my testing (previous version) it reduced the size of the rendered app.js by 21.39kb.

If you keep the PropTypes that can be expexted.
Why not using https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types?

[stephenburgess8]

I see, so I assume that since I was in a development env that's why I was seeing the difference in size. If it were in production then all PropTypes would be removed?

In that case, seems like the change would be purely conceptual as you say. In that case, it seems like the majority including Dan Abramov are using

import React, {Component, PropTypes} from 'react' as per this discussion: https://discuss.reactjs.org/t/es6-import-as-react-vs-import-react/360/2

[oliviertassinari]

@stephenburgess8 Considering this comment, doing this change sounds like a good idea.

I just import React, { Component, PropTypes } because JSX wants React to be in scope. Once this is solved at some point, I won't even need React.

[stephenburgess8]

@nathanmarks Could you do this automatically?

[nathanmarks]

Yup, will do it tonight or tomorrow

[mbrookes]

@stephenburgess8 No need - a couple of global regex search-and-replaces will do the trick.

[nathanmarks]

@mbrookes That's automating it 😄