Some modules actually rely on propTypes

[mp31415]

Hi,

thank you for the plugin. It works well. I noticed though that some of the RN 3rd party modules rely on propTypes property. See for example react-native-vector-icons\lib\icon-button.js. Removing propTypes then results in

AndroidRuntime: com.facebook.react.common.JavascriptException: 
Requested keys of a value that is not an object., stack:

I modified my version of the plugin, so that only files not residing in node_modules are cleaned up preventing this type of crashes. In a function like isModule below I check if it's my own file or not and only then remove propTypes:

	function isModule(scope) {
		if (!scope.hub.file.opts) {
			return true;
		}
		var filename = scope.hub.file.opts.filename;
		if (!filename) {
			return true;
		}
		return filename.indexOf("node_modules") !== -1;
	}

I thought maybe it's worth adding an extra option to the plugin that would allow to skip 3rd party modules modification if needed.

[oliviertassinari]

Regarding the big picture, I'm wondering if it should be plugin responsibility to know there the transformation is going to be applied. Shouldn't it be in the babel config?
I'm also wondering if letting Babel parse the node_modules is the most used approach. Many people just parse their source code and ignore that folder.

Still, that could be a good option for this plugin.

[mp31415]

I see. So many tools - so little time. I grabbed whatever .babelrc is suggested somewhere in react native docs, which is pretty much empty:

{
  "presets": ["react-native"]
}

Then I added some plugins to cleanup the release build a bit:

{
  "presets": ["react-native"],
  "env": {
	"production": {
	  "plugins": ["transform-remove-console", "transform-react-remove-prop-types"]
	}
  }
}

I don't care about any cleanup for the node_modules stuff, but to build the final bundle a lot of modules go through babel transformations and both plugins above one way or another break stuff. I modified my version of the both plugins and reported the changes. I have very little experience with Babel ecosystem, so no opinion whether the plugin must be modified in any way or it should be left as an exercise to the reader :) But directly applying both plugins breaks release builds for many people.

[oliviertassinari]

I have very little experience with Babel ecosystem

I don't have much either, I couldn't find any plugin implementing an ignore option.
I'm gonna ask @hzoo his point of view on the issue as an important member of the Babel community. I wish he's gonna have some time to answer it.

Otherwise, implementing an ignore option with an API / behavior close to the global on http://babeljs.io/docs/usage/cli/ sounds like a good idea.
Removing propTypes can be dangerous with the node_modules as you don't control how they are used by lib authors.
For instance, react-bootstrap runtime relies on it.

[mrdulin]

@oliviertassinari so, maybe it's unnecessary to remove propTypes.Because It's dangerous with node_modules.I think the network bandwidth saved can be ignored.

[oliviertassinari]

So, maybe it's unnecessary to remove propTypes. Because It's dangerous with node_modules

I have added Is it safe? section in the README.

Yes, it can be dangerous with the node_modules. That's not something I would be doing without a
continuous integration test suite.

[oliviertassinari]

Implementing an eslint rule sounds like a good path going forward. We have that proposal jsx-eslint/eslint-plugin-react#696.

[hzoo]

Commented there, if it's easy to detect you can just use something like throw path.buildCodeFrameError("error here")

[oliviertassinari]

I'm gonna start documenting breaking patterns I have seen here.
Hopefully, it's gonna help fixing that issue.

K.O

Reference in render from an import

import { pick } from 'lodash';
import SomeComponent from './SomeComponent';

export default function AnotherComponent(props) {
  const passedProps = pick(props, Object.keys(SomeComponent.propTypes));
  return (
    <SomeComponent {...passedProps} />
  );
};

Reference in propTypes from an import

import {SomeComponent} from './SomeComponent';

const propTypes = {
  value: SomeComponent.propTypes.value,
};

export default function AnotherComponent(props) {
  return <SomeComponent value={props.value} />;
}

AnotherComponent.propTypes = propTypes;

O.K.

React native

import {View} from 'react-native';

class CustomView extends React.Component {
  static propTypes = {
    style: View.propTypes.style
  };
  render() {
    return (
      <View style={this.props.style}>..</View>
    );
  }
}

I think that a good first iteration would be to look for MemberExpression referencing a propTypes from an ImportDeclaration.
Once we have that heuristic, how do we notifies users?
The buildCodeFrameError sounds like a good idea. Thanks for bringing that up 👍 .

I think that an eslint rule is a complementary solution.
Avoiding leaking propTypes between components could be considered healthy for the codebase as close to inheritance.

[oliviertassinari]

This Babel plugin now throws when encountering the two previous dangerous pattern.
The Is it safe? section of the documentation exposes some workaround.

[oliviertassinari]

Even react-router is affected we need a better solution than just documenting it.

[timdorr]

A configurable glob pattern of modules to ignore might work. I'm willing to add in some guard comments around that statement (in react-router), if it would help.

[oliviertassinari]

A configurable glob pattern of modules to ignore might work.

@timdorr I agree, I think that I'm gonna add an option to skip files inside a node_modules.

I'm willing to add in some guard comments around that statement

A simple refactorisation can allow avoiding the issue. Are you opened to that option? I can make a PR.

[timdorr]

PRs > Issues. Go right ahead and I'll look at merging it and and pushing out a patch release.

[rosskevin]

@oliviertassinari - we transform all our node_modules for production so that we can take advantage of ES2015+ code and tree shaking, so I decided we might as well use this plugin there and just filter anomalies.

I've been having a hard time ignoring react-router though, what am I missing?

        ["transform-react-remove-prop-types", {
          "ignoreFilenames": ["Router.js"] //["node_modules/react-router"]
        }],

[rosskevin]

Nevermind, my release is behind. I was getting this through babel-preset-react-optimize but it is using an old release. I'll switch to this dependency directly.

[oliviertassinari]

Notice that the react-router fix hasn't been released yet.

[rosskevin]

Yes, I'm on V3 though (due to relay) and looking to just ignore that one file. BTW - looks like babel-preset-react-optimize isn't being maintained and references this project.