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
Remove unused MemberExpression root identifiers #158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
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.
4a9e0c5
to
7efede2
Compare
const somePropTypes = process.env.NODE_ENV !== "production" ? { | ||
foo: PropTypes.string, | ||
bar: PropTypes.number | ||
} : {};; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know where the ;;
is coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately no. I suspect it might be a bug in Babel, or maybe with the way we are doing the templating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK thanks, @eps1lon might have found something.
In our codebase, I noticed that we had the following pattern that was
not getting properly cleaned up:
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 offoo.bar
from being collected. However, inthis case, we actually want the
foo
part offoo.bar
to be collectedfor 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
infoo.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.