JSX - ts.findNextToken() does not account for "JsxText" case

[JamesHenry]

TypeScript Version:

1.8.2

Background:

I have been working on trying to get https://github.com/eslint/typescript-eslint-parser, a TypeScript parser plugin for ESLint, off the ground, and the current job is adding JSX support.

ESLint uses espree and so the aim of the project is to convert the output of the tsc to an AST which espree expects. We already have a solid suite of JSX tests to develop against (taken from the espree project itself).

We do make use of some helper TypeScript helper methods when converting the AST, such as ts.findNextToken() and I believe I may have found an issue with the way it handles JsxText

Given this JSX code:

<div>@test content</div>;

...the TSC produces the AST we expect - with @test content represented as a single JsxText node:

However, when iterating through the AST using ts.findNextToken(), rather than a single JsxText node, we get a node for @, test and content separately.

Upon quick inspection of the source I noticed that the related function, ts.findPrecedingToken(), was doing an early check for a JsxText node type, whereas ts.findNextToken() wasn't.

(See https://github.com/Microsoft/TypeScript/blob/master/src/services/utilities.ts#L338 vs https://github.com/Microsoft/TypeScript/blob/master/src/services/utilities.ts#L364)

Sure enough when I monkey patched the relevant bit of typescript.js to exit early if n.kind was JsxText, a single JsxText token was recognised for @test content, and my test passed.

If you agree that this is a bug, then I would be very happy submit a PR, otherwise any tips on how I might have arrived at this false positive would be very much appreciated!

[RyanCavanaugh]

Seems like a bug to me.

[DanielRosenwasser]

As a note, this is actually not part of the public API and is an internal implementation detail.

For the record, we've special cased handling of JSX Text in other places in the language service, so ideally any changes here would be leveraged by the rest of the language service.

[RyanCavanaugh]

Should we be marking these @internal ?

[nzakas]

We are relying on these methods in typescript-eslint-parser, so please don't hide them from us. :)

[DanielRosenwasser]

@RyanCavanaugh it is marked @internal, it's in src/services/utilities.ts.

[JamesHenry]

I appreciate that it is has been marked as internal, but hopefully you guys can appreciate the challenge of transforming the tsc AST means we really need the help of these utilities functions.

We have made some great progress on the project already, and it would be so awesome to be able release this parser to unlock the power of ESLint for TypeScript users.

I am not sure how familiar you guys are with ESLint and its community, but for me it is one of the finest open source projects out there, and one of the most useful tools I have ever used. The configurability and plugin architecture is immense!

Perhaps in the medium term, we could work with you guys to align on some "public" utility methods which the typescript-eslint-parser would make use of? What do you think?

As I say, I am happy to submit the a PR for this issue.

P.S. Mine is the merely the voice of the passionate TS and ESLint user, whereas ESLint is very much @nzakas project, so his opinion on any of the above trumps mine 100x :)

[DanielRosenwasser]

@JamesHenry eslint is a terrific project, but committing to a public API should not be done flippantly. What exactly do you need findNextToken for?

[nzakas]

@DanielRosenwasser the purpose of typescript-eslint-parser is to convert the output from the TypeScript engine into a format that ESLint can consume. To do that, we convert the AST into an ESTree-compatible one and convert the tokens into Esprima-compatible versions. Both require us to look at syntax on the token level to account for the discrepancies between the formats. You can see this process here:
https://github.com/eslint/typescript-eslint-parser/blob/master/lib/ast-converter.js

Plus, the docs seem to indicate that anything on ts is considered a public API:
https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API

I started from this page and, by inspecting that object, found the other methods. Undoubtedly I'm not the only one who went through this process and didn't even look at the annotated source code. I think relying on @internal in the source code to mark things you don't want API consumers to use isn't very effective. I'd suggest going through the extra effort of obscuring method names or removing them from public interfaces to ensure developers don't rely on things you don't want them to use.

I'm happy to use something else if you want. These language utilities are fantastic and it's what makes ESLint support for TypeScript even remotely possible. What you have works great for our purposes, aside from this bug. :)

[DanielRosenwasser]

I think relying on @internal in the source code to mark things you don't want API consumers to use isn't very effective. I'd suggest going through the extra effort of obscuring method names or removing them from public interfaces to ensure developers don't rely on things you don't want them to use.

Hmm, @mhegazy, we should discuss this. We should probably make it more explicit what is and isn't supported. I'm actually noticing portions of our API that we'd previously reviewed which are not marked as internal to begin with.

@nzakas Node#getChildren() would get you what you want too, and I believe that is publicly supported.

[nzakas]

@DanielRosenwasser thanks, we can try using that.

Just for completeness, these are the things we currently rely on:

1. ts.findNextToken
2. ts.getAncestor
3. ts.findChildOfKind
4. ts.NodeFlags
5. ts.SyntaxKind

Aside from the first, are these all "public"?

[DanielRosenwasser]

I think the last two are fair game, but I don't believe the others actually are. One rule of thumb is that if it's in either utilities.ts file, then it's private. Otherwise, it will need an /* @internal */

[nzakas]

Unfortunately, we can't stop using these methods because there is no easy way to emulate them.

Out of curiosity, why would you consider these methods internal-only? They seem like fairly standard operations (we expose similar to ESLint rules) that many people would need if they want to work with the TypeScript AST at all. Since they are already public and we are probably not the only project using them, wouldn't a better option be to "bless" these APIs or else provide an equivalent that you can commit to maintaining?

[thomas-darling]

So this issue is quite old, but we need the ts.findChildOfKind method too, and find it quite strange that it's not part of the official API and its the type definitions. As mentioned before, this, and maybe a few other, methods seem like fairly standard operations, that are kinda hard to avoid.

Would you consider making them official, and adding them to your type definitions?

[JamesHenry]

I don't feel right representing this issue any more, it has been too long since I reported it or it was relevant for me.

As Daniel mentions above, I believe the public Node#getChildren() got us what we needed in the end. You can follow the linked issues etc to see what fixes/changes were applied.