Fix path.remove() leading & trailing comments sharing

[dmail]

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

[mention-bot]

@dmail, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo and @existentialism to be potential reviewers.

[babel-bot]

Hey @dmail! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

• Node v7 - linux
• Node v6 - linux
• Node v4 - linux
• Node v7 - linux

[babel-bot]

Hey @dmail! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

• Node v7 - linux
• Node v6 - linux
• Node v4 - linux
• Node v7 - linux

[babel-bot]

Hey @dmail! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

• Node vnode - linux

[codecov]

Codecov Report

Merging #5504 into 7.0 will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##              7.0    #5504      +/-   ##
==========================================
+ Coverage   85.31%   85.43%   +0.12%     
==========================================
  Files         200      200              
  Lines        9490     9512      +22     
  Branches     2696     2702       +6     
==========================================
+ Hits         8096     8127      +31     
+ Misses        894      886       -8     
+ Partials      500      499       -1

Impacted Files	Coverage Δ
packages/babel-traverse/src/path/comments.js	93.1% <100%> (-2.73%)	⬇️
...ckages/babel-core/src/transformation/file/index.js	87.09% <0%> (-0.06%)	⬇️
...in-transform-es2015-template-literals/src/index.js	100% <0%> (ø)	⬆️
packages/babel-traverse/src/path/lib/hoister.js	90.9% <0%> (+0.18%)	⬆️
packages/babel-types/src/validators.js	84.95% <0%> (+0.27%)	⬆️
...bel-plugin-transform-es2015-classes/src/vanilla.js	90.17% <0%> (+0.42%)	⬆️
packages/babel-types/src/converters.js	91.03% <0%> (+0.75%)	⬆️
packages/babel-traverse/src/path/replacement.js	74.33% <0%> (+0.88%)	⬆️
packages/babel-traverse/src/path/modification.js	74.75% <0%> (+0.97%)	⬆️
... and 5 more

---

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 baafe48...cf2c912. Read the comment docs.

[babel-bot]

Hey @dmail! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

• Node vnode - linux

[dmail]

Sorry for the commits noise. This pull request is still missing specific tests.

[babel-bot]

Hey @dmail! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

• Node v7 - linux
• Node v6 - linux
• Node v4 - linux
• Node v7 - linux
• Node vnode - linux

[babel-bot]

Hey @dmail! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

• Node vnode - linux

[dmail]

I won't commit more now all checks are passing.

At the moment, this pull request prevents comments order being messed up when using path.remove(). However, tests added by this PR demonstrates that path.remove() still have a negative impact on comment outputs.

I cannot go further because of how comments are stored in the tree. I mean leadingComments & trailingComments properties on nodes does not seems to be sufficient to preserve comment when a node is removed.

Let me know if this pull request helps you,
Cheers.

[hzoo]

Yeah we may have to redo comments based on how eslint/prettier are handling them

[hzoo]

looks good,

Made dmail-fork#1 for fixture tests instead of js

[dmail]

Warning : Travis says tests are passing but TEST_ONLY=babel-traverse make test is currently failing.

I'm noticing that your comments/attachment/expected.js are expecting correct comment outputs.

For instance you're expecting

hasPrev;
/* top */
/* left */willRemove;/* right */
/* bottom */

To become

hasPrev;
/* top */
/* left */ /* right */
/* bottom */

However the current output is

hasPrev; /* top */ /* left */ /* right */ /* bottom */

[hzoo]

@dmail it works locally for me (both make test-only and TEST_ONLY)

oh I had the same problem originally: it's because if you just npm install it installs the old version rather than the version of babel-traverse that's linked. If you make bootstrap again it should work

[hzoo]

Truly awesome work @dmail! thanks 👏