search

eslint / typescript-eslint-parser

An ESLint custom parser which leverages TypeScript ESTree to allow for ESLint to lint TypeScript source code.
Other
915 stars 92 forks source link

New: Pass services generated by typescript-estree to consumer #568

Closed uniqueiniquity closed 5 years ago

uniqueiniquity commented 5 years ago

This PR picks up the changes made in https://github.com/JamesHenry/typescript-estree/pull/24 and exposes them to consumers of this package. In particular, this will allow rules to reference the TS program created to parse the TS source, as well as provide maps between corresponding nodes in the TS and ESTree representations of the parsed ASTs.

In this PR, I have conservatively introduced a new parser options generateServices so that this functionality is only provided when opted-in. Note that the project option to typescript-estree serves the same purpose.

uniqueiniquity commented 5 years ago

@JamesHenry for review as well

platinumazure commented 5 years ago

Hi! Apologies, been meaning to review but haven't had time. The changes look sensible here, but I want to play with this in a local branch as well :smile:

We'll be grateful for James's review as well, if he can make the time, but he isn't a blocker here.

kaicataldo commented 5 years ago

@platinumazure Does this look good to you? If so, can we merge this and do a major release?

armano2 commented 5 years ago

ok, i did some testing, a lot of testing and i found one issue with this. https://github.com/bradzacher/eslint-plugin-typescript/pull/230

i'm having issues with getting type out of es5 method parseFloat

and this is not a case with defined already defined methods

right now typescript-estree is not supporting this, but it should allow to specify custom compiler options.

uniqueiniquity commented 5 years ago

As mentioned in https://github.com/bradzacher/eslint-plugin-typescript/pull/230, I'm not really a fan of opening up the API of typescript-estree to accept compiler options. The TypeScript compiler works very closely with the file system itself, so it can produce strange behavior when working with files that don't actually exist. The issue that @armano2 is seeing appears to be related to not being able to resolve lib files when an invalid file name (in the current test runner, <input>) is being linted. The TypeScript test infrastructure handles this by providing a virtual file system, which seems heavy-handed for this scenario. However, @armano2 also found that just providing an empty file as a concrete location allows library lookup to succeed, so if that's not too hacky, I'd support that as a way to enable testing semantic lint rules. What do y'all think? @kaicataldo @platinumazure

uniqueiniquity commented 5 years ago

Quick follow up: I'm looking into the issue that @armano2 raised, specifically to better support scenarios where the file being linted is not naturally included by a provided tsconfig.json (e.g. Vue files, test files). The issue will probably be addressed in typescript-estree.

uniqueiniquity commented 5 years ago

@platinumazure @kaicataldo I was able to resolve the issues that @armano2 pointed out, and the fix is included in typescript-estree@7.0.1. However, I think v7.0.0 included some non-trivial breaking changes to the AST output.

Should I try to resolve those issues as part of this PR, or should the typescript-estree upgrade and associated baseline changes come in a subsequent PR?

armano2 commented 5 years ago

@uniqueiniquity upgrade to 7.0.0 is done already in separated PR #584, looks like it will have to be merged first

platinumazure commented 5 years ago

Just checking on the status of this. I think we still need to upgrade typescript-estree to at least 7.0.1 and then this PR can do any necessary rebasing and tweaks before this is ready for merge?

@uniqueiniquity Would you mind testing this PR against the changes raised in #589 to make sure there aren't any unexpected regressions? It is likely that #589 will be merged before this. Thanks!

uniqueiniquity commented 5 years ago

@platinumazure Sure thing!

uniqueiniquity commented 5 years ago

@platinumazure Seems all fine to me. My changes to actual behavior are more general than the changes in #589 and my tests happen to not use any of the AST types that changed. I'm able to successfully run the rules that I've written that use parserServices (i.e., type information) after merging in the changes from #589.

uniqueiniquity commented 5 years ago

Happy new year, all! Is there anything I can do to help this get merged?

JamesHenry commented 5 years ago

Thanks so much for your patience @uniqueiniquity, I raised it with the team again 👍

JamesHenry commented 5 years ago

Looks like the build is failing right now just FYI

armano2 commented 5 years ago

its required changes from https://github.com/eslint/typescript-eslint-parser/pull/584

JamesHenry commented 5 years ago

@uniqueiniquity I'm back to reviewing PR's on this side of the fence again :) Feel free to ping me any time on Twitter to discuss something or ping on a PR/issue.

Let's get #596 merged and then get everything updated for this one

JamesHenry commented 5 years ago

596 has been merged, once this is rebased against the latest, we'll get this in as well :)

kaicataldo commented 5 years ago

Closing this PR since the project has been moved to the TypeScript ESLint organization. Feel free to reopen the PR there.