Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dirkholsopple opened this issue Aug 24, 2018 · 7 comments

Comments

@dirkholsopple
Copy link

dirkholsopple commented Aug 24, 2018

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
Copy link
Collaborator

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 added a commit that referenced this issue Aug 24, 2018
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

Since the minifier will remove any unused function expressions for us,
we don't need to jump through that hoop. There is a possibility that
there are some references inside that function that could be further
cleaned up by us that then minifier won't catch, but I'm not really sure
it's worth worrying about since this seems pretty edge-casey. If we want
to go down that path, instead of skipping over these nodes, we would
probably want to remove them but in the wrap mode replace it with a
function that returns an object instead of an object.

Fixes #153
@lencioni
Copy link
Collaborator

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

@dirkholsopple
Copy link
Author

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
Copy link
Collaborator

@dirkholsopple any luck with a more complete example?

@dirkholsopple
Copy link
Author

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
Copy link

swarthy commented Aug 30, 2018

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 added a commit that referenced this issue Sep 19, 2018
I have been investigating a bug that occurs when a ClassExpression is
using the experimental class properties for propTypes. I am not sure
what the best fix for it is yet, but I wanted to get up a failing test
case to help showcase the bug.
@lencioni
Copy link
Collaborator

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 (#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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants