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

Avoid removing variable assignments that are functions #154

Closed
wants to merge 1 commit into from

Conversation

lencioni
Copy link
Collaborator

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

cc @dirkholsopple

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

Successfully merging this pull request may close these issues.

1 participant