Skip to content
This repository has been archived by the owner on Jan 14, 2019. It is now read-only.

AST missing start and end values. #19

Open
HauptmannEck opened this issue Oct 24, 2018 · 6 comments
Open

AST missing start and end values. #19

HauptmannEck opened this issue Oct 24, 2018 · 6 comments

Comments

@HauptmannEck
Copy link

What version of TypeScript are you using?
3.1.3
What version of typescript-estree are you using?
2.1.0 (I have not seen any changes in the code that would fix this in v3)
What code were you trying to parse?

import * as React from 'react';

const Thing = ( { imgSrc } ) => (
    <img src={imgSrc}
         className="class"
    />
);

What did you expect to happen?
Generate AST that matched the ESlint AST so that I could use eslint-plugin-react rules.
What actually happened?
The AST does not have start or end numerical values so any rule fixes that use them damages the code.

I see there are comments in the alignment tests that allude to this being on purpose:

 * - Remove "start" and "end" values from Babylon nodes to reduce unimportant noise in diffs ("loc" data will still be in
 * each final AST and compared).

The @babel/parse and Espree parsers both have the start and end values on their AST output, so I am curious as to why it was chosen to not have them in this projects AST.

This is the rule that I am seeing breakage on for example:
https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/rules/jsx-first-prop-new-line.js#L48

@JamesHenry
Copy link
Owner

Interestingly, those properties are not actually defined in the ESTree spec: https://github.com/estree/estree/blob/master/es5.md

I noticed esprima doesn't have them, but most of the other popular parsers do. We can probably consider adding them in as it is very non-disruptive, but I would note that it is not necessarily a design goal of this parser to produce ASTs to comply with assumptions in ESlint rules. The goal is to take TypeScript source code and produce an ESTree-compatible AST.

@HauptmannEck
Copy link
Author

Luckily as long as the range values exist there is a work around and we can change the code consuming the AST to not use the start/end. Although I noticed range also doesn't seem to be a part of the ESTree spec either.

I don't have an opinion on if the fields should be added or not, just wanted to get the conversation out there. Hopefully this issue can help others in the future understand the cause of their bugs.

@mysticatea
Copy link

In fact, ESLint AST spec also doesn't have start and end properties. Espree accidentally introduced those properties when it switched to acorn from esprima. It has been left because removing properties is a heavy operation.

@kaicataldo
Copy link
Collaborator

Given that this isn't part of the ESTree spec, is this something we want to do? It sounds like it could just be left to consumers to implement if it's something they need.

@JamesHenry
Copy link
Owner

Is there any easy way to offer a compatibility layer within typescript-eslint-parser, so that eslint rules don't need to be rewritten?

I imagine post-processing the whole AST is an option, but may be quite slow?

@mysticatea
Copy link

ESLint core rules have never used start and end properties :)
But I'm not sure plugins...

I think that we can add the verification to check whether rules don't use start/end into RuleTester. But it will be a breaking change.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants