fix: Ensure correct scope after traversal #283
Conversation
This fixes a bug that can be seen when istanbul coverage is enabled for a babel@7 project which uses reference replacement, such as `babel-plugin-lodash`. The root issue is that plugins like these assume that scope references will always be Identifiers but Istanbul may replace an identifier with a SequenceExpression. In babel@7 this replacement overwrites the existing scope reference and later plugin scope reference replacement is at best unsafe and at worst may result in an invalid AST. This seems like a pretty unfortunate situation, and ideally Babel automatically maintains these scope references during transforms or the many packages using scope would account for this situation. The pattern I see in other first-class babel plugins prefer to re-crawl scope after a transform which can affect identifiers (example: https://github.com/babel/minify/blob/master/packages/babel-plugin-minify-dead-code-elimination/src/index.js#L969) To follow that example, I'm proposing this fix that crawls the scope after the path traversal - ensuring any subsequent references to scope get correct references. I tested this in a project that used istanbul via jest along with babel-plugin-lodash that suffered from this issue and found that with this patch the issue was resolved and the inspected compiled output looked correct.
|
This is difficult. From @loganfsmyth on babeljs slack (in response to someone else asking about
So to move forward I think I'm going to need a minimal test-case. Preferably without jest (sorry jest adds a lot of moving parts). Something which uses |
|
Closing in favor of istanbuljs/babel-plugin-istanbul#192 |
This fixes a bug that can be seen when istanbul coverage is enabled for a babel@7 project which uses reference replacement, such as
babel-plugin-lodashorbabel-plugin-ramda.The root issue is that plugins like these assume that scope references will always be Identifiers but Istanbul may replace an identifier with a SequenceExpression. In babel@7 this replacement overwrites the existing scope reference and later plugin scope reference replacement is at best unsafe and at worst may result in an invalid AST.
This seems like a pretty unfortunate situation, and ideally Babel automatically maintains these scope references during transforms or the many packages using scope would account for this situation. The pattern I see in other first-class babel plugins prefer to re-crawl scope after a transform which can affect identifiers (example: https://github.com/babel/minify/blob/master/packages/babel-plugin-minify-dead-code-elimination/src/index.js#L969)
To follow that example, I'm proposing this fix that crawls the scope after the path traversal - ensuring any subsequent references to scope get correct references.
I tested this in a project that used istanbul via jest along with babel-plugin-lodash that suffered from this issue and found that with this patch the issue was resolved and the inspected compiled output looked correct.