Add forbid-foreign-prop-types rule #1055

iancmyers · 2017-01-31T20:00:18Z

People may want to use babel-plugin-transform-react-remove-prop-types to remove propTypes from their components in production builds, as an optimization. The forbib-foreign-prop-types rule forbids using another component's prop types unless they are explicitly imported/exported, which makes that optimization less prone to error.

I've included the use-cases described in #696 as tests. I'd be happy to add additional test cases if recommended.

Fixes #696

lencioni · 2017-01-31T20:03:24Z

docs/rules/forbid-foreign-prop-types.md

@@ -0,0 +1,28 @@
+# Forbid foreign propTypes (forbid-foreign-prop-types)
+
+This rule forbids using another component's prop types unless they are explicitly imported/exported. This allows people who want to use babel-plugin-transform-react-remove-prop-types to remove propTypes from their components in production builds, to do so safely.


It would be nice to link to the babel plugin here.

lencioni · 2017-01-31T20:06:58Z

docs/rules/forbid-foreign-prop-types.md

+The following patterns are not considered warnings:
+
+```js
+import {propTypes: someComponentPropTypes}, SomeComponent from './SomeComponent';


Unfortunately, even importing like this does not ensure that propTypes is explicitly exported from the module (and not just a property on the exported object).

What do you think about making this rule additionally ensure that propTypes is explicitly exported when being imported as well? Alternatively, if there is a rule in eslint-plugin-import that covers this, we should mention it here.

Also, this line should be

import SomeComponent, {propTypes as someComponentPropTypes} from './SomeComponent';

@lencioni Looks like eslint-plugin-import has the named rule which would ensure that named imports are also named exports: https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/named.md

This is great! Will you please add a note about this and a link to the rule in the documentation?

ljharb · 2017-01-31T21:50:59Z

lib/rules/forbid-foreign-prop-types.js

+      },
+
+      ObjectPattern: function(node) {
+        var propTypesNode = node.properties.find(function(property) {


eslint-plugin-react still supports node 0.10, so please use array.prototype.find here instead (after this is merged, i'll update #1038 to switch back to .find)

ljharb · 2017-01-31T21:51:49Z

tests/lib/rules/forbid-foreign-prop-types.js

+  }
+};
+
+require('babel-eslint');


none of the code in this test requires object rest/spread or babel-eslint; could these tests just stick with espree?

ljharb · 2017-01-31T21:52:11Z

lib/rules/forbid-foreign-prop-types.js

+      MemberExpression: function(node) {
+        if (node.property && node.property.type === 'Identifier' && node.property.name === 'propTypes' ||
+            node.property && node.property.type === 'Literal' && node.property.value === 'propTypes') {
+          context.report({


it'd be helpful if the error could include the position of the warning, so that editors can jump the cursor right to it

lencioni · 2017-01-31T22:52:16Z

tests/lib/rules/forbid-foreign-prop-types.js

+  }, {
+    code: 'import { propTypes as someComponentPropTypes } from "SomeComponent";',
+    parser: 'babel-eslint'
+  }],


It might be worth adding some valid cases like:

const foo = propTypes foo(propTypes) foo + propTypes const foo = [propTypes] const foo = { propTypes }

And maybe even junk like this (note the lowercase):

foo.propTypes foo['propTypes'] const { propTypes } = foo

I'm not sure why lowercase matters - I agree that cases where it's an object or array literal should be valid, but class foo extends React.Component should have the same restrictions (someone could always import Foo from './foo' and use it just fine)

I think this plugin already has some component detection logic we could reuse?

I was thinking lowercase should perhaps be allowed because in React, components must be uppercase. Lowercase means DOM element. But this distinction is unlikely to matter much here. Reusing component detection logic sounds good to me.

oliviertassinari · 2017-02-09T20:01:49Z

@iancmyers Thanks for working on that!
I have tried an iteration at the babel plugin with no luck.

react-native is encouraging the following pattern. We might need an exception.

import {View} from 'react-native';

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

ljharb · 2017-02-09T20:04:12Z

That's just static class properties (which are a stage 2 proposal) - definitely those should be covered as well, but since it's just sugar for CustomView.propTypes = …, it shouldn't be difficult.

iancmyers · 2017-02-14T19:16:50Z

@lencioni @ljharb I've taken another pass at this and addressed much of your feedback.

People may want to use babel-plugin-transform-react-remove-prop-types to remove propTypes from their components in production builds, as an optimization. The `forbib-foreign-prop-types` rule forbids using another component's prop types unless they are explicitly imported/exported, which makes that optimization less prone to error. Fixes jsx-eslint#696

ljharb

LGTM

ljharb · 2017-02-14T19:47:22Z

lib/rules/forbid-foreign-prop-types.js

+    // --------------------------------------------------------------------------
+
+    function isLeftSideOfAssignment(node) {
+      return node.parent.type === 'AssignmentExpression' && node.parent.left === node;


will a node always have a .parent property?

I believe that MemberExpression and ObjectPattern will always at least have a parent of Program.

lencioni · 2017-02-14T21:05:45Z

@iancmyers thanks for your contribution!

@oliviertassinari Thanks for raising this scenario. I'm going to merge this as it is, and we can follow up with an improvement to address that if we need to. One thought is to allow referencing propTypes on objects imported from packages, but we can discuss the details in a separate issue or PR.

lencioni reviewed Jan 31, 2017

View reviewed changes

iancmyers force-pushed the icm-forbid-foreign-prop-type branch from d7f5e49 to f5cc025 Compare January 31, 2017 21:10

ljharb requested changes Jan 31, 2017

View reviewed changes

lencioni reviewed Jan 31, 2017

View reviewed changes

iancmyers force-pushed the icm-forbid-foreign-prop-type branch 2 times, most recently from 66e8e24 to 6241f4b Compare February 14, 2017 18:59

lencioni approved these changes Feb 14, 2017

View reviewed changes

iancmyers force-pushed the icm-forbid-foreign-prop-type branch from 6241f4b to 0b25b66 Compare February 14, 2017 19:23

ljharb approved these changes Feb 14, 2017

View reviewed changes

lencioni merged commit 54abb8b into jsx-eslint:master Feb 14, 2017

renovate-bot mentioned this pull request Feb 16, 2017

Update dependency eslint-plugin-react to version 6.10.0 singapore/lint-condo#240

Merged

oliviertassinari mentioned this pull request Apr 15, 2017

Instead of removing propTypes, replace with empty object oliviertassinari/babel-plugin-transform-react-remove-prop-types#95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add forbid-foreign-prop-types rule #1055

Add forbid-foreign-prop-types rule #1055

iancmyers commented Jan 31, 2017

lencioni Jan 31, 2017

lencioni Jan 31, 2017

iancmyers Jan 31, 2017

lencioni Jan 31, 2017

ljharb Jan 31, 2017

ljharb Jan 31, 2017

ljharb Jan 31, 2017

lencioni Jan 31, 2017

ljharb Feb 2, 2017

ljharb Feb 2, 2017

lencioni Feb 2, 2017

oliviertassinari commented Feb 9, 2017 •

edited

ljharb commented Feb 9, 2017

iancmyers commented Feb 14, 2017

ljharb left a comment

ljharb Feb 14, 2017

iancmyers Feb 14, 2017

lencioni commented Feb 14, 2017

		@@ -0,0 +1,28 @@
		# Forbid foreign propTypes (forbid-foreign-prop-types)

		This rule forbids using another component's prop types unless they are explicitly imported/exported. This allows people who want to use babel-plugin-transform-react-remove-prop-types to remove propTypes from their components in production builds, to do so safely.

Add forbid-foreign-prop-types rule #1055

Add forbid-foreign-prop-types rule #1055

Conversation

iancmyers commented Jan 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Feb 9, 2017 • edited

ljharb commented Feb 9, 2017

iancmyers commented Feb 14, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lencioni commented Feb 14, 2017

oliviertassinari commented Feb 9, 2017 •

edited