Fix: Optimize convertTokens, treat JsxText as token (fixes #70)

[JamesHenry]

UPDATE: I was able to use this PR to work around the long-standing issue with JsxText nodes outlined in #70

The test case: jsx/test-content has been enabled.

---

Based on feedback from the TypeScript team here: microsoft/TypeScript#13519 (comment)

I will be looking into refactoring remaining occurrences of ts.findNextToken in future too

[soda0289]

Does this fix the issue with JSX? I was tracking that issue down. It's a bug in findNexToken() where it does not descend into JSX elements. I was going to submit pull request for that once I got a clean solution but this looks like it avoids it.

[JamesHenry]

No, was also hoping that, this issue is still present on this branch

[soda0289]

That sucks, I guess this JSX bug runs a little deeper.
This is still a great optimization since it doesn't have to start at the top of the AST every time we find a token, like it did with findNextToken().

[eslintbot]

Thanks for the pull request, @JamesHenry! I took a look to make sure it's ready for merging and found some changes are needed:

• Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[JamesHenry]

@soda0289 It is, but it's a task that is only performed once, so the benefits are minimal.

I think I may have just added a solution for the JsxText issue - now that we have control over the traversal into child nodes, we can create an exception for JsxText and treat it like a token.

I am going to double check the behaviour on some JSX now and if it works I'll be converting this PR into a bug fix

[soda0289]

Hmm I think JsxText is a token.
The main examples i was test are:

<test></test>

<test> </test>

<test>
</test>

<test> aaa
</test>

[JamesHenry]

So you are agreeing with me? 😄

I have confirmed that the parser will no longer throw errors for whitespace in JsxText, going to turn this into a bug fix.

[soda0289]

Nice!! I'm very excited for JSX support!

I thought isToken would return true for JsxText isn't that extra check unnecessary?

[soda0289]

I just checked the typescript code. In ts version 2.0 JsxText is not a token:
https://github.com/Microsoft/TypeScript/blob/release-2.0/lib/typescript.d.ts#L283
In ts master it is a token:
https://github.com/Microsoft/TypeScript/blob/master/lib/typescript.d.ts#L50

[JamesHenry]

Was just replying that they do not classify it as a token 😄 Nice digging though! We can remove that extra check in the 2.2 branch probably, but it will not do any harm

[eslintbot]

LGTM

[JamesHenry]

Updated the description, this is now ready for review.

[eslintbot]

LGTM

[soda0289]

I tested it out. Works great, Thanks!

[DanielRosenwasser]

This sounds like a bug on us. I vaguely recall trying to fix this months ago but it was not a simple fix if it's the same issue I was thinking about - basically there were some invariants violated in the language service as a result.

[JamesHenry]

Interesting, and thanks for keeping an eye on the project @DanielRosenwasser 😄

I will take another look at this once TypeScript 2.2 hits stable.

[JamesHenry]

@DanielRosenwasser just FYI this does indeed work as of TS 2.2!

I am removing this extra check in the PR to support 2.2 -> #169

[DanielRosenwasser]

👍

[JamesHenry]

Throwing this open for anyone on @eslint/eslint-team to do a final review. I think this should be pretty safe to merge