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

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

Closed
wants to merge 7 commits into from

Conversation

uniqueiniquity
Copy link
Contributor

This PR picks up the changes made in JamesHenry/typescript-estree#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 uniqueiniquity changed the title Pass services generated by typescript-estree to consumer New: Pass services generated by typescript-estree to consumer Nov 26, 2018
@uniqueiniquity
Copy link
Contributor Author

@JamesHenry for review as well

@platinumazure
Copy link
Member

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 😄

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

parser.js Outdated Show resolved Hide resolved
Copy link
Member

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ben and I had lunch the other day in Seattle to discuss this so I knew it was coming :)

Great work, Ben!

@kaicataldo kaicataldo added breaking This change is backwards-incompatible and removed enhancement labels Dec 14, 2018
@kaicataldo
Copy link
Member

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

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! (Sorry for the delay!)

@armano2
Copy link
Contributor

armano2 commented Dec 15, 2018

ok, i did some testing, a lot of testing and i found one issue with this.
bradzacher/eslint-plugin-typescript#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
Copy link
Contributor Author

As mentioned in bradzacher/eslint-plugin-typescript#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
Copy link
Contributor Author

uniqueiniquity commented Dec 20, 2018

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
Copy link
Contributor Author

@platinumazure @kaicataldo
I was able to resolve the issues that @armano2 pointed out, and the fix is included in [email protected]. 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
Copy link
Contributor

armano2 commented Dec 27, 2018

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

@platinumazure
Copy link
Member

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
Copy link
Contributor Author

@platinumazure Sure thing!

@uniqueiniquity
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@JamesHenry
Copy link
Member

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

@JamesHenry
Copy link
Member

Looks like the build is failing right now just FYI

@armano2
Copy link
Contributor

armano2 commented Jan 10, 2019

its required changes from #584

@JamesHenry
Copy link
Member

@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
Copy link
Member

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

@kaicataldo
Copy link
Member

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

@kaicataldo kaicataldo closed this Jan 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking This change is backwards-incompatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants