fix(typescript-estree): jsx comment parsing

[nathanmarks]

Fixes #643
Fixes #648
Fixes #841

The problem is that with the fix performed in #607, when you have something like this:

const pure = () => {
  return (
      <Foo
        // one
        foo={123}
        // two
        bar="woof"
      />
  );
}

We currently perform reScanJsxToken() on the closing brace for the foo attribute value.

This results in a SyntaxKind.JsxText token like this:

"}\n        // two\n        bar="woof"\n      />\n  );\n}\n"

So the // two never gets correctly identified. If we hadn't performed the reScanJsxToken() on the } closing brace for the JSX expression, the // two would have been tokenized correctly.

I have fixed this by changing the rescan call added in the PR referenced earlier to only apply if the JSX expression is inside a JSX element (which addresses the case it was added to fix to begin with).

Honestly, this feels like a bit of a band-aid solution. Admittedly, I don't have a lot of experience with the typescript scanner but I feel as though this is the best solution without reworking the method a bit. Looking at some of the tokens we end up with after rescanning sometimes, there seems to be some sacrifices made in order to keep this as simple as possible and just locate the comments (which is a totally fair and valid design decision).

I feel like a better way to make these rescan decisions would be to move forward to the next meaningful token before making the decision to rescan. Like, if we move forward past the } in this example:

const fifth = <div>{}http://example.com</div>;

And then look at the AST for the next token (http which ends up as SyntaxKind.Identifier without rescanning), the AST will tell us that we're on a JSXText node. So if we know the proceeding token was a closing brace for a JSXExpression, we can check the AST to see if we're on a JSXText node before making the decision to rescan. Even if the next token is a whitespace or a new line, the AST we have will already know that's a JSXText node.

I think this is a better approach than basing it on the type of parent for the JSXExpression (the change I made to fix the issue in this PR), since the issue is that we don't want the jsx text nodes to get tokenized incorrectly (which is what resulted in the text after http: getting incorrectly picked up as a comment). We could also probably benefit from this approach for the > token logic too.

If you want, I can update this PR and use the technique described above instead of the current fix I have applied.

Anyways, I have no experience working with the TS scanner output so I could be way off in my conclusions here, but the fact that we end up with tokens identified as JsxText that look like }http://example.com (notice the } included) also indicates that we're a little off in the approach here (although one can argue that for the sake of extracting the comments it's irrelevant).

[JamesHenry]

Thanks a lot for the detailed write up @nathanmarks. The main priority I would have before merging this is ensuring that it is not going to cause any regressions in Prettier.

Please could you try applying the changes in your PR to Prettier to double check? From memory there is a fairly comprehensive set of tests for comments in JSX over there...

[nathanmarks]

Thanks a lot for the detailed write up @nathanmarks. The main priority I would have before merging this is ensuring that it is not going to cause any regressions in Prettier.

Please could you try applying the changes in your PR to Prettier to double check? From memory there is a fairly comprehensive set of tests for comments in JSX over there...

will do shortly and report back! just had a super busy week,

[nathanmarks]

@JamesHenry

With my local @typescript-eslint/typescript-estree yarn linked. All tests are passing on prettier/prettier@4b96097

yarn test

Test Suites: 926 passed, 926 total
Tests:       6070 passed, 6070 total
Snapshots:   4778 passed, 4778 total
Time:        53.132s
Ran all test suites.
✨  Done in 54.35s.

yarn test-integration

Test Suites: 45 passed, 45 total
Tests:       940 passed, 940 total
Snapshots:   549 passed, 549 total
Time:        16.094s
Ran all test suites matching /tests_integration/i.
✨  Done in 17.32s.

[codecov]

Codecov Report

Merging #703 into master will increase coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #703      +/-   ##
==========================================
+ Coverage   94.21%   94.33%   +0.12%     
==========================================
  Files         112      111       -1     
  Lines        4821     4593     -228     
  Branches     1336     1268      -68     
==========================================
- Hits         4542     4333     -209     
+ Misses        159      149      -10     
+ Partials      120      111       -9

Impacted Files	Coverage Δ
packages/typescript-estree/src/convert-comments.ts	96.29% <ø> (ø)	⬆️
packages/eslint-plugin/src/util/misc.ts	80.95% <0%> (-7.05%)	⬇️
...-plugin/src/rules/explicit-member-accessibility.ts	88.23% <0%> (-1.35%)	⬇️
packages/eslint-plugin/src/rules/array-type.ts	91.35% <0%> (-1.28%)	⬇️
...s/experimental-utils/src/eslint-utils/deepMerge.ts	93.33% <0%> (-0.79%)	⬇️
...nt-plugin/src/rules/indent-new-do-not-use/index.ts	98.39% <0%> (-0.01%)	⬇️
packages/eslint-plugin/src/rules/index.ts	100% <0%> (ø)	⬆️
packages/parser/src/parser.ts	100% <0%> (ø)	⬆️
packages/eslint-plugin/src/rules/ban-types.ts	100% <0%> (ø)	⬆️
packages/eslint-plugin-tslint/src/custom-linter.ts	100% <0%> (ø)	⬆️
... and 37 more

[JamesHenry]

Awesome, thanks for verifying @nathanmarks!

[nathanmarks]

@JamesHenry did you have any thoughts RE the other points i brought up?

Maybe it's fine how it is, I just don't want to deep dig a hole that will need more branching and digging in the future to handle edge cases like this.