Fix issue semi-colon gets inserted unnecessarily

[buunguyen]

Q	A
Patch: Bug Fix?	yes
Major: Breaking Change?	no
Minor: New Feature?	no
Deprecations?	no
Spec Compliancy?	yes
Tests Added/Pass?	yes
Fixed Tickets	Fixes #5744
License	MIT
Doc PR	no
Dependency Changes	no

Original:

if (true) [a, b] = [b, a];

Transpiled version:

if (true) {
  ;
  var _ref = [b, a];
  a = _ref[0];
  b = _ref[1];
  _ref;
}

The problem is insertAfter could push an empty expression statement, resulting in the semi-colon. This change checks to make sure the expression statement is not empty before pushing.

[codecov]

Codecov Report

Merging #5749 into 7.0 will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##              7.0    #5749      +/-   ##
==========================================
- Coverage   84.63%   84.62%   -0.01%     
==========================================
  Files         282      282              
  Lines        9861     9862       +1     
  Branches     2766     2766              
==========================================
  Hits         8346     8346              
  Misses        997      997              
- Partials      518      519       +1

Impacted Files	Coverage Δ
packages/babel-traverse/src/path/modification.js	73.07% <0%> (-1.69%)	⬇️
packages/babel-traverse/src/path/context.js	87.06% <0%> (+0.86%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce6f672...a0df33b. Read the comment docs.

[hzoo]

Is it better to do ^ or just check if it's an not an EmptyExpression?

[buunguyen]

I can't find EmptyExpression or isEmptyExpression in the codebase. Or did you mean we should create a new method isEmptyExpression to wrap the above? Like:

if (this.node && !this.isEmptyExpression()) {...}
...
isEmptyExpression() => this.isExpressionStatement() && this.node.expression == null

[buunguyen]

Or you probably meant EmptyStatement :). Let me try that.

[hzoo]

oh sorry I thought it was "EmptyStatement"

[buunguyen]

I just tried, it doesn't work.

In this particular case, this.node is of type ExpressionStatement. this.isEmptyStatement() returns false because ExpressionStatement isn't EmptyStatement and they aren't aliases of one another.

I wondered why this.node wasn't EmptyStatement in the first place and dug in a bit more. Turns out this line https://github.com/babel/babel/blob/7.0/packages/babel-traverse/src/path/replacement.js#L50 sets this.container['expression'] = null and this.container is this.node in the piece of code in question.

[hzoo]

And do we want to make the same change in insertBefore?

[buunguyen]

I thought so too, but couldn't come up with a test for that case. Basically we need something that results in:

{
  code 
  code 
  ;  <--  empty statement here
}

Without a test/bug that exposes the above behavior, there's no guarantee such situation is possible. Currently, the behavior is caused by that line to this.container['expression'] = null, which is only invoked before insertAfter. I haven't seen anything line that before insertBefore. That said, I could definitely make that change to insertBefore for the extra caution. Let me know.

[hzoo]

Ah ok I see. I guess this.container['expression'] = null is rather a weird way to do things but we can do this

[hzoo]

lgtm was just curious about how we should do the check

[hzoo]

Thanks for looking into this and doing a through investigation!