Breaking: Infer parsing as JSX from file extension (fixes #399)

[kaicataldo]

fixes #399

This is a WIP implementation of inferring whether the parser should parse in JSX mode or not based on the file extension of the file being parsed. Wanted to have a place to start a discussion, and then when we decide how we want to proceed finish this up.

This is a breaking change for users who are running ESLint on files (and therefore have a filepath). However, I can't think of a reason why you'd ever want the behavior to be different.

That being said, I believe that we do need to keep the behavior to set it manually for when a filepath is not provided, since we don't know whether it should be parsed in JSX mode or not. Example:

echo 'var foo: number = 1;' | ./node_modules/.bin/eslint --stdin --no-eslintrc --parser-options='ecmaFeatures:{jsx: true}' --parser typescript-eslint-parser

A few questions I have:

• I'm not very knowledgable about other downstream consumers of this parser - will this impact them negatively? In other words, should this only happen when { isParseForESLint: true }, or would this be a welcome change for others in the community as well?
• To make it safer, we could have the passed in config always override the inferred value, however, as I mentioned, I can't think of a situation where this would be the desirable behavior. Doing so would also make existing configurations that have this set continue to do the wrong thing.
• I don't see a way around making this a breaking change, since even if we override it the behavior could be different for someone who does not have parserOptions.ecmaFeatures.jsx defined in their configuration. How does the team feel about this?

[shawnrice]

The reasons not to distinguish ts from tsx are the same reasons found in discussions about a jsx file extension. For a lively debate, see the discussion at: airbnb/javascript#985.

I can understand having a lint rule that requires the tsx file extension, but making it part of the parser effectively enables what should be a lint rule, and so it does violate a separation of concerns between the tool and its configuration.

[kaicataldo]

@shawnrice It's actually not quite the same thing. In TypeScript, .tsx is a requirement for using JSX in your code (unlike when compiling with, say, Babel). See the official docs for more details.

[corbinu]

@kaicataldo

I think that this point breaking changes are pretty expected here as things are still moving pretty fast. I at least am totally ok with this.

[JamesHenry]

Very cool, thanks for exploring this @kaicataldo!

@azz I think prettier will only benefit from this, right? We could potentially get rid the of the heuristics in https://github.com/prettier/prettier/blob/master/src/language-js/parser-typescript.js

Prettier is the only known programmatic consumer at this point.

[azz]

Currently it is possible to use Prettier without passing a file path:

cat foo.tsx | prettier --parser typescript 

So we'd have to keep the try tsx then ts behaviour at least in that case, but it would be a welcome improvement.

[kaicataldo]

How are we feeling about this?

[JamesHenry]

Other than updating it to be a breaking change I think it is good to go!

[kaicataldo]

Sorry I lost track of this. Besides for updating the commit message/PR title, what else needs to be done to update it to a breaking change?

Also, it looks like snapshot tests are failing. My naive guess is that it has something to do with filename extensions of fixture files, but I'll take a look and see what's going on.

[azz]

If the snapshots fail it's still a breaking change (see my comment above)

[kaicataldo]

@azz Mind clarifying what you mean? I thought we decided this was a breaking change.

[JamesHenry]

@kaicataldo Since this was first created I added a lot more sophisticated integration testing: https://github.com/eslint/typescript-eslint-parser#integration-tests

Please could you add some for this? You can ping me in ESLint chat if anything is unclear

[JamesHenry]

@kaicataldo If you are interested in reopening this against the new project, please feel free, otherwise I will happily reimplement this myself of there.

Thanks again!