Skip to content

Commit

Permalink
Avoid removing variable assignments that are functions
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lencioni committed Aug 24, 2018
1 parent 7c17eb2 commit c53e402
Show file tree
Hide file tree
Showing 6 changed files with 106 additions and 0 deletions.
8 changes: 8 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,14 @@ export default function(api) {
return
}

if (
path.node.init.type === 'FunctionExpression' ||
path.node.init.type === 'ArrowFunctionExpression'
) {
removedPaths.add(path)
return
}

const { referencePaths } = path.scope.getBinding(path.node.id.name)

// Count the number of referencePaths that are not in the
Expand Down
22 changes: 22 additions & 0 deletions test/fixtures/variable-assignment/actual.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,3 +68,25 @@ const propTypesCreateClass = { foo: PropTypes.string };
const FooCreateClass = createReactClass({
propTypes: propTypesCreateClass
});

const propTypesFunction = function() {
return {
foo: PropTypes.string,
};
};

const FooFunction = () => (
<div />
);

FooFunction.propTypes = propTypesFunction();

const propTypesArrowFunction = () => ({
foo: PropTypes.string,
});

const FooArrowFunction = () => (
<div />
);

FooArrowFunction.propTypes = propTypesArrowFunction();
20 changes: 20 additions & 0 deletions test/fixtures/variable-assignment/expected-remove-es5.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,23 @@ var FooExported = function FooExported() {
var FooCreateClass = createReactClass({
displayName: "FooCreateClass"
});

var propTypesFunction = function propTypesFunction() {
return {
foo: PropTypes.string
};
};

var FooFunction = function FooFunction() {
return React.createElement("div", null);
};

var propTypesArrowFunction = function propTypesArrowFunction() {
return {
foo: PropTypes.string
};
};

var FooArrowFunction = function FooArrowFunction() {
return React.createElement("div", null);
};
14 changes: 14 additions & 0 deletions test/fixtures/variable-assignment/expected-remove-es6.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,17 @@ export const propTypesExported = {
const FooExported = () => <div />;

const FooCreateClass = createReactClass({});

const propTypesFunction = function () {
return {
foo: PropTypes.string
};
};

const FooFunction = () => <div />;

const propTypesArrowFunction = () => ({
foo: PropTypes.string
});

const FooArrowFunction = () => <div />;
24 changes: 24 additions & 0 deletions test/fixtures/variable-assignment/expected-wrap-es5.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,27 @@ var FooCreateClass = createReactClass({
displayName: "FooCreateClass",
propTypes: propTypesCreateClass
});

var propTypesFunction = function propTypesFunction() {
return {
foo: PropTypes.string
};
};

var FooFunction = function FooFunction() {
return React.createElement("div", null);
};

FooFunction.propTypes = process.env.NODE_ENV !== "production" ? propTypesFunction() : {};

var propTypesArrowFunction = function propTypesArrowFunction() {
return {
foo: PropTypes.string
};
};

var FooArrowFunction = function FooArrowFunction() {
return React.createElement("div", null);
};

FooArrowFunction.propTypes = process.env.NODE_ENV !== "production" ? propTypesArrowFunction() : {};
18 changes: 18 additions & 0 deletions test/fixtures/variable-assignment/expected-wrap-es6.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,21 @@ const propTypesCreateClass = process.env.NODE_ENV !== "production" ? {
const FooCreateClass = createReactClass({
propTypes: propTypesCreateClass
});

const propTypesFunction = function () {
return {
foo: PropTypes.string
};
};

const FooFunction = () => <div />;

FooFunction.propTypes = process.env.NODE_ENV !== "production" ? propTypesFunction() : {};

const propTypesArrowFunction = () => ({
foo: PropTypes.string
});

const FooArrowFunction = () => <div />;

FooArrowFunction.propTypes = process.env.NODE_ENV !== "production" ? propTypesArrowFunction() : {};

0 comments on commit c53e402

Please sign in to comment.