istanbuljs / babel-plugin-istanbul

A babel plugin that adds istanbul instrumentation to ES6 code
BSD 3-Clause "New" or "Revised" License
624 stars 73 forks source link

[regression] [5.1.2] variable override #201

Closed pi0 closed 5 years ago

pi0 commented 5 years ago

This issue introduced with 5.1.2 (5.1.1 and below are fine)

Reproduction repo: https://github.com/pi0/babel-plugin-istanbul-bug (Use yarn test)

Reverting this change by #192 fixes problem.

Details

When using @babel/preset-env:

import path from 'path'

function test() {
  const _path = 'test'
  return path.relative(__dirname, _path)
}

Transpiles into: (Correct)

var _path2 = _interopRequireDefault(require("path"));

function test() {
  const _path = 'test';
  return _path2.default.relative(__dirname, _path);
}

Using babel-plugin-istanbul@5.1.2: (Bug)

var _path = _interopRequireDefault(require("path"));

function test() {
  cov_1523mz691.f[0]++;

  const _path = (cov_1523mz691.s[0]++, 'test');

  cov_1523mz691.s[1]++;
  return _path.default.relative(__dirname, _path);
}

Using babel-plugin-istanbul@5.1.1 (Correct)

var _path2 = _interopRequireDefault(require("path"));

function test() {
  cov_1523mz691.f[0]++;

  const _path = (cov_1523mz691.s[0]++, 'test');

  cov_1523mz691.s[1]++;
  return _path2.default.relative(__dirname, _path);
}
coreyfarrell commented 5 years ago

I was able to further isolate this issue with the provided repository by replacing @babel/preset-env with @babel/plugin-transform-modules-commonjs. What I don't know yet is if we've exposed a bug in the babel ESM -> CJS transform plugin, or if it's inappropriate for us to call path.scope.crawl().

@leebyron unfortunately we may have to revert your patch. I did a quick experiment and was able to get babel-plugin-lodash working with babel-plugin-istanbul@5.1.1 by adding path.scope.crawl() to the end of the ImportDeclaration.exit() function at https://github.com/lodash/babel-plugin-lodash/blob/7d2342a3b849c3c8ef3b2d904b7e2394205466ff/src/index.js#L84. I'm not sure if this change is actually appropriate or just shifts the issue over there.

coreyfarrell commented 5 years ago

I've just pushed changes to revert #192 and add a regression test to verify that babel-plugin-istanbul and @babel/plugin-transform-modules-commonjs play nice together. I'll try to push an updated release shortly.

@leebyron until we can find a new solution to #192 you will need to pin your dependency to babel-plugin-istanbul@5.1.2.

coreyfarrell commented 5 years ago

babel-plugin-istanbul@5.1.3 is now released.