Skip to content

Commit

Permalink
Remove unused MemberExpression root identifiers (#158)
Browse files Browse the repository at this point in the history
In our codebase, I noticed that we had the following pattern that was
not getting properly cleaned up:

```js
const shapePropType = PropTypes.shape({
  foo: PropTypes.string,
});

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

ComponentA.propTypes = {
  foo: shapePropType.isRequired,
};
```

The notable thing here is that inside the propTypes assignment, a
MemberExpression is used instead of just an identifier. However, in our
visitor for collecting nested identifiers, we special-cased Identifiers
that have a MemberExpression parent to not be collected. This was to
prevent the `bar` portion of `foo.bar` from being collected. However, in
this case, we actually want the `foo` part of `foo.bar` to be collected
for possible additional cleanup, so we need to add a little more logic
to make this happen.

To solve this, I added a function that takes a path and traverses up the
tree until it finds the first non-MemberExpression, and then traverses
down the left side of that tree until it finds the root identifier. This
should be the `foo` in `foo.bar.baz`, for instance.

It seems likely that there is a better more built-in Babel way to do
this, but I was unable to find anything to point me in that direction so
I rolled my own.
  • Loading branch information
lencioni committed Sep 20, 2018
1 parent 5fa6824 commit 3c78036
Show file tree
Hide file tree
Showing 7 changed files with 200 additions and 2 deletions.
37 changes: 37 additions & 0 deletions src/index.js
Expand Up @@ -65,6 +65,35 @@ function areSetsEqual(set1, set2) {
return !Array.from(set1).some(item => !set2.has(item))
}

function memberExpressionRootIdentifier(path) {
// Traverse up to the parent before the topmost member expression, and then
// traverse back down to find the topmost identifier. It seems like there
// might be a better way to do this.
const parent = path.findParent(p => !p.isMemberExpression())
const { type } = parent.node

let memberExpression
if (type === 'ObjectProperty') {
// The topmost MemberExpression's parent is an object property, so the
// topmost MemberExpression should be the value.
memberExpression = parent.get('value')
}

if (!memberExpression) {
// This case is currently unhandled by this plugin.
return null
}

// We have a topmost MemberExpression now, so we want to traverse down the
// left half untli we no longer see MemberExpressions. This node will give us
// our leftmost identifier.
while (memberExpression.node.object.type === 'MemberExpression') {
memberExpression = memberExpression.get('object')
}

return memberExpression.get('object')
}

export default function(api) {
const { template, types, traverse } = api

Expand All @@ -74,6 +103,12 @@ export default function(api) {
Identifier(path) {
if (path.parent.type === 'MemberExpression') {
// foo.bar

const root = memberExpressionRootIdentifier(path)
if (root) {
nestedIdentifiers.add(root.node.name)
}

return
}

Expand Down Expand Up @@ -188,6 +223,7 @@ export default function(api) {
}
},
},

// Here to support stage-1 transform-class-properties.
ClassProperty(path) {
const { node, scope } = path
Expand All @@ -205,6 +241,7 @@ export default function(api) {
}
}
},

AssignmentExpression(path) {
const { node, scope } = path

Expand Down
2 changes: 0 additions & 2 deletions test/fixtures/create-class/expected-remove-es6.js
@@ -1,5 +1,3 @@
var createReactClass = require('create-react-class');

var PropTypes = require('prop-types');

createReactClass({});
39 changes: 39 additions & 0 deletions test/fixtures/variable-assignment-member-expressions/actual.js
@@ -0,0 +1,39 @@
const shapePropType = PropTypes.shape({
foo: PropTypes.string,
});

const ComponentA = () => <div />;
ComponentA.propTypes = {
foo: shapePropType.isRequired,
};

const somePropTypes = {
foo: PropTypes.string,
bar: PropTypes.number,
};

const ComponentB = () => <div />;
ComponentB.propTypes = {
foo: somePropTypes.foo.isRequired,
};

const somePropTypesC = {
foo: PropTypes.string,
bar: PropTypes.number,
};

const ComponentC = () => <div />;
ComponentC.propTypes = {
foo: somePropTypesC['foo'].isRequired,
};

const somePropTypesD = {
foo: PropTypes.string,
bar: PropTypes.number,
};

const ComponentD = () => <div />;
const foo = { bar: 'foo' };
ComponentD.propTypes = {
[foo.bar]: somePropTypesD['foo'].isRequired,
};
@@ -0,0 +1,21 @@
"use strict";

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

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

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

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

var foo = {
bar: 'foo'
};
@@ -0,0 +1,11 @@
const ComponentA = () => <div />;

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

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

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

const foo = {
bar: 'foo'
};
@@ -0,0 +1,50 @@
"use strict";

var shapePropType = process.env.NODE_ENV !== "production" ? PropTypes.shape({
foo: PropTypes.string
}) : {};;

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

ComponentA.propTypes = process.env.NODE_ENV !== "production" ? {
foo: shapePropType.isRequired
} : {};
var somePropTypes = process.env.NODE_ENV !== "production" ? {
foo: PropTypes.string,
bar: PropTypes.number
} : {};;

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

ComponentB.propTypes = process.env.NODE_ENV !== "production" ? {
foo: somePropTypes.foo.isRequired
} : {};
var somePropTypesC = process.env.NODE_ENV !== "production" ? {
foo: PropTypes.string,
bar: PropTypes.number
} : {};;

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

ComponentC.propTypes = process.env.NODE_ENV !== "production" ? {
foo: somePropTypesC['foo'].isRequired
} : {};
var somePropTypesD = process.env.NODE_ENV !== "production" ? {
foo: PropTypes.string,
bar: PropTypes.number
} : {};;

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

var foo = {
bar: 'foo'
};
ComponentD.propTypes = process.env.NODE_ENV !== "production" ? babelHelpers.defineProperty({}, foo.bar, somePropTypesD['foo'].isRequired) : {};
@@ -0,0 +1,42 @@
const shapePropType = process.env.NODE_ENV !== "production" ? PropTypes.shape({
foo: PropTypes.string
}) : {};;

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

ComponentA.propTypes = process.env.NODE_ENV !== "production" ? {
foo: shapePropType.isRequired
} : {};
const somePropTypes = process.env.NODE_ENV !== "production" ? {
foo: PropTypes.string,
bar: PropTypes.number
} : {};;

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

ComponentB.propTypes = process.env.NODE_ENV !== "production" ? {
foo: somePropTypes.foo.isRequired
} : {};
const somePropTypesC = process.env.NODE_ENV !== "production" ? {
foo: PropTypes.string,
bar: PropTypes.number
} : {};;

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

ComponentC.propTypes = process.env.NODE_ENV !== "production" ? {
foo: somePropTypesC['foo'].isRequired
} : {};
const somePropTypesD = process.env.NODE_ENV !== "production" ? {
foo: PropTypes.string,
bar: PropTypes.number
} : {};;

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

const foo = {
bar: 'foo'
};
ComponentD.propTypes = process.env.NODE_ENV !== "production" ? {
[foo.bar]: somePropTypesD['foo'].isRequired
} : {};

0 comments on commit 3c78036

Please sign in to comment.