-
-
Notifications
You must be signed in to change notification settings - Fork 75
Document the tradeoffs for this approach #61
Comments
Thanks for checking out the project @alexeagle! I am not sure I understand you correctly when you say "I'm curious why you'd want to lint a down-transpiled version of the code". This project parses the source using the TypeScript compiler to produce a standard TypeScript AST, and then simply converts that AST into a form which matches the ESTree spec (evolving over at https://github.com/ESTree/estree). This allows ESLint to interpret the AST and means we can take advantage of its (awesome) existing infrastructure, which includes a solid auto-fixing story, plugins and, of course, rules. Now that the foundations of this parser are in, we are beginning to capture more and more type information and this will continue to evolve into custom rules based on it. |
PS. I am already using this parser on my large Angular 1 app, which is written in TypeScript, using decorators from a project I contribute to called ng-metadata I am one of a whole generation of Angular 1 developers who are long time ESLint users. One of the reasons I am so passionate about this project is because I do not believe changing linter (and moving away from the amazing ESLint) should be part of a decision to migrate from Angular 1 to Angular 2. |
PPS @mgechev's work on linting against the style-guide has been fantastic, and I definitely believe it will be possible to match those capabilities through custom rules in ESLint + typescript-eslint-parser |
@alexeagle I think that @JamesHenry hit the nail precisely. One of the things we've heard repeatedly from ESLint users is that they are upset that moving to TypeScript means leaving ESLint behind. We've also heard that they wish TSLint was more like ESLint. There's no good reason that ESLint can't support TypeScript, all that was missing was a parser. Take the case of another type dialect, Flow. Flow developers use ESLint directly with either Babel or the Flow parser, and with packages like |
I see, thanks for clarifying. But, you still won't have a type checker available to ESLint. We have started writing TSLint checks that are type-aware, for example checking the types of the two operands to |
Please could you point us to a specific example of what you mean? |
Check out this test: TSLint can warn if you add a string and a number, or a string and null, based on the type of the two operands. You can't do this kind of check syntactically. To make this possible, we added an input to TSLint that points to a |
Thanks, that is clear and an interesting case. I will defer to @nzakas on this one as it relates directly to ESLint and rule config 😄 All the pieces are there, and it would be awesome if we could bring them together without needing big changes to ESLint |
I feel like the ESLint plugin could potentially do this (for example, we have literally just started one here https://github.com/nzakas/eslint-plugin-typescript). It could look up the project's tsconfig.json in the same way that TSLint does to make a http://eslint.org/docs/developer-guide/working-with-rules Perhaps we may need to keep a reference to the original TSNode in final output to prevent reimplementing a bunch of logic though... |
I have obviously hacked this together in a matter of minutes so it won't be a bullet-proof approach. In order to benefit from all of the existing TypeChecker methods I simply modded the parser to keep a reference to the original TSNode. @nzakas How do you feel about that as a way of enabling these kinds of checks? The great thing is that this does not require any changes to ESLint or how rules are configured. Everything is encapsulated in the parser and plugin. ESLint Rule code (pretty much taken from @alexeagle's code in TSLint): Created TS program for access to TypeChecker like so: It's 00:16 here so officially signing off! |
We could do that, though I'd want to tweak the implementation a bit. @alexeagle I think the overall point here is that ESLint is a lot more flexible than it may seem at first. Im willing to bet that ESLint can do anything TSLint can do or wants to do if we really focus on it. |
@nzakas that's great, thank you for explaining. My remaining concern is just about fragmentation. Since there is no common API for writing a check/rule among the various tools, they don't have feature parity, and there are many editors to wire up, we risk having no tool that includes all the checks and works in all the editors. This leaves teams to use multiple tools and forgo good tooling support. ESLint is likely the best supported among editors (is this true?) so do we expect there will be enough effort to port remaining rules and features? To make this a bit more concrete, we are working on the TS extensibility-model branch to add support for Angular 2 templates to the language service. This means you could ask for completions inside an Angular expression in a .html file, for example. But it also means we can write lint checks that detect style or correctness violations in templates, or in code that spans TS/html boundary. We will have to pick just one tool to build this support on. So far @mgechev has been the furthest on this track with his codelyzer tool, which was forked from TSLint. So Angular team has some interest in implementing our stuff in the right tool, rather than having to do it N times. |
Just a comment that I am currently switching a large ES6 codebase to Typescript and lack of rules in TSlint has been an issue. We have a rather large eslint config and maybe a 2/3rds of the rules were available in TSLint even when using https://www.npmjs.com/package/tslint-eslint-rules The bigger issue to me though is the lack of ability to use eslint plugins of which there are many more for example we use https://github.com/wix/eslint-plugin-lodash So I am really excited at this approach. Thanks for all the hard work! |
@alexeagle thanks for the background info. While I can understand the concern about fragmentation, we are still planning to proceed here. From my point of view, TSLint is always going to be playing catch-up to ESLint in terms of rules and features. In the meantime, we have a fairly simple path forward to get some really good TypeScript support into ESLint. I think there's also a good opportunity to figure out how to extend ESLint to allow things like directly using the TypeScript services (we already have plugins, after all, so it's just a matter of figuring out how to extend plugins to do it). I was planning on reaching out to the TSLint team at some point to see if we could coalesce the projects into one, as I think there are benefits to both sides (they have the TS experience, we have rules and plugins). So, I can't really tell you which way to go for Angular. In my experience, the ESLint community tends to build what it needs so long as the capabilities are there. That's what this project is about: giving the community the capability to use TypeScript with ESLint and then see what flows from that. |
FYI: Proposal for how to expose additional services related to a parser in ESLint has been filed here: eslint/eslint#6974 |
@JamesHenry What sort of work do you anticipate would have to go into the current ESLint rules to make them work well with TS? For example, I noticed you said at one point:
Do you mean for TS specific-features? Obviously a rule having to deal with, say, disallowing type aliases would be part of a custom set and would make no sense as part of core ESLint. Or do you mean that there would have to be a second version of each ESLint rule for TS? Say, for example, we want to require semicolons after a type alias statement. Would this be built in to the current If one core set of ESLint rules could be maintained that work on JS and TS, it would be a big win for TS users and we wouldn't need to duplicate effort on making similar rules between ESLint and TSLint. But if rules are still going to need two separate implementations, then benefits are more minimal |
Essentially, the TypeScript source gets turned into an ESTree AST. This is what espree (the default parser for ESLint) produces, and so it is what ESLint core rules are built on. This means that it should be possible for the vast majority, if not all, of the core ESLint rules to just work with TypeScript when using this parser. I am currently using Having this in place was the first major milestone of the The completion of the work on decorators brings us closer to the second major milestone - additionally capture all of the TypeScript type and esnext syntax in the final ESTree AST. (We have closely replicated how Flowtype records those nodes in its AST for that). This where the complementary project eslint-plugin-typescript comes in. It will be responsible for implementing new rules which work on these extra "type nodes". As you can see, @nzakas has already implemented a Here you can see an example of how this all fits together: The core ESLint rule "semi" is used in tandem with the TypeScript specific syntax rule of "type-annotation-spacing" (which is made available by the plugin). We will improve the general documentation around this very soon, but hopefully that has helped clarify things for now. |
Thanks for the detailed response @JamesHenry. That sounds like the ideal situation to me - having all (or at least most) of ESLint's core rules work on TypeScript and having some additional rules that deal with TS-specific things. However, I'm still uncertain about the answer to one of my original questions. Say we have the following TS code: type Result<T> = Success<T> | Failure
const someResult: Result<T> = getResultFromSomewhere() And also say we're using the core |
@jkillian you can try that right now and see for yourself. :) |
Right, the first line would not be picked up by core ESLint "semi" (the second would), because none of the node types are encapsulated in that rule definition. Currently, (please correct me if I am wrong @nzakas) there is no direct way to extend an existing rule with new behaviour, so the |
That's correct. That doesn't mean we can't make it work, though. We can try to make ESLint smarter in a bunch of ways so we don't have to duplicate rules. |
TSLint actually has an advantage here - since TS is a superset of JS, it can apply its rules to JS and TS files alike without modifications to the rules. (Although at the moment, you can't actually use TSLint on JS files - but there's no practical reason for this and it'll be a relatively simple change to teach it to lint JS files.) Thanks for creating #77 @JamesHenry, I think this is a key point of discussion in figuring out if ESLint can support TS in a first-class way. I'm interested to see where that discussion goes! |
@jkillian, here is an example of ESLint "semi" picking up on the type alias declaration without any plugins or extra rules: As you might have seen on #77, I am running well over 100 ESLint core rules on my large TypeScript codebase now using this parser. Thanks a lot for flagging the type alias case, we're really motoring now! 😄 |
I'm very pleased to see this work and discussion with TSLint folks. Having decided to move to TS for my libraries and components. To my mind while TSLint is great it has along way to go to catch up with the reach and community of ESLint. I have 2 main use cases that mean TSLint is really not an option
PS I am not an angular user. |
Closing this issue since its been open for a while and I think has been answered. |
Awkward issue - but having worked on TSLint lately and wiring it into our toolchain (and we'll start adding angular2 template support soon, + @mgechev ) - I'm curious why you'd want to lint a down-transpiled version of the code. You lose type information of course, plus all the ES7+ features (Esp. decorators) are de-sugared so they're harder to comprehend and analyze.
But I imagine you have a good reason to do this. Too much work to re-implement the checks in TSLint? I imagine adding some utilities to make it much easier to express a certain predicate about an AST subtree...
The text was updated successfully, but these errors were encountered: