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

Remove unnecessary React propTypes from the production build. :balloon:
MIT License
897 stars 61 forks source link

Incorrect code is generated for prop-type created through a function call when using mode: "wrap" #153

Open dirkholsopple opened 6 years ago

dirkholsopple commented 6 years ago

Starting with 0.4.15, incorrect code is generated when using the mode: "wrap" setting for a prop-type like this

const generateType = (x) => {
  return PropTypes.oneOf([x-1, x, x+1]);
};

Component.propTypes = {
  prop: generateType(1)
};

The code generated for the prop-type is

const generateType = process.env.NODE_ENV !== "production" ? x => {
  return PropTypes.oneOf([x - 1, x, x + 1]);
} : {};;

which will result in an error when NODE_ENV === "production" because {} is not a function.

A working example of the bug can be found here: https://github.com/dirkholsopple/prop-type-removal-bug-reproduction

lencioni commented 6 years ago

Ah good find! I suppose in this case, we can just leave that line alone and minification should be able to clean it up.

Would you be interested in putting up a PR to fix this, or if not that at least a failing test case that should pass once the fix is in place?

lencioni commented 6 years ago

Actually, in your example, this shouldn't produce an error when NODE_ENV is production because the function is never called, right?

dirkholsopple commented 6 years ago

Yes, that would be the case in this example. The component where I found this was doing something more complex with prop-types that resulted in the prop-types not being completely removed so the function was invoked when importing the file. I'll see if I can come up with a more complete example

lencioni commented 6 years ago

@dirkholsopple any luck with a more complete example?

dirkholsopple commented 6 years ago

Was finally able to narrow down what was causing the prop-type definition not to be removed. It seems to be some kind of interaction between the prop-type removal and babel-plugin-transform-class-properties when used from webpack. I updated the example above accordingly

swarthy commented 6 years ago

Another example

class MyComponent extends Component {
  static propTypes = {
    children: props => {
      const children = React.Children.toArray(props.children)
      for (let i = 0; i < children.length; i++) {
        const childType = children[i].type
        if (childType !== Column && !(childType.prototype instanceof Column)) {
          return new Error('ReduxTable only accepts children of type Column')
        }
      }
    }
  }
  // ...
}

failed with error:

ERROR in ./src/components/MyComponent.js 151:85

Module parse failed: Unexpected token (151:85)

You may need an appropriate loader to handle this file type.

|     const children = process.env.NODE_ENV !== "production" ? React.Children.toArray(props.children) : {};;
| 
>     for (let i = process.env.NODE_ENV !== "production" ? 0 : {};; i < children.length; i++) {
|       const childType = process.env.NODE_ENV !== "production" ? children[i].type : {};;
| 

output for contains 4 statement

for(a;b;c;d;} {
  ...
}
lencioni commented 6 years ago

I see, I think these are two separate issues.

@dirkholsopple your issue seems related to wrap mode skipping over propTypes removal for static properties on ClassExpressions. I put up a failing test case for this here (https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types/pull/159), but I'm not sure how to best solve it. Any help would be appreciated.

@swarthy your issue seems related to traversing and removing identifiers found in already removed code when using wrap mode. Can you open a separate issue for this?