Interface imports not parsed out

[mightyiam]

What version of TypeScript are you using?
See package.json.

What version of typescript-eslint-parser are you using?
See package.json.

What code were you trying to parse?

See repository:
https://github.com/mightyiam/cyclejs-web-components/pull/13

What did you expect to happen?

Interface imports should have been parsed out.

What happened?

They seem to have remained, because I got:
https://travis-ci.org/mightyiam/cyclejs-web-components/jobs/207631830#L658, for example.

[soda0289]

I don't have a lot of free time to dig into this. Could you post a short code sample that reproduces the failure. The output in travis does not say what rule is failing so it is hard to diagnose the problem. Could you use eslint from the command line and post the output from there.

If I had to guess it looks like the no-unused-vars or no-undef rule. Both of these rules are known to not currently work due in part to the way eslint and escope traverse the AST. You can check out this issue #77 for more information on rules that do not work with this parser. There are some workaround such as using the typescript compiler options noUnusedLocals and noUnusedParameters. There is also a rule in the typescript-eslint-plugin but it does not cover all cases.

[quantuminformation]

For no-unused-vars I am getting false positives for valid ts code:

import { IGameUnit } from "../gameUnits/IGameUnit";

which is used:

any ideas?

[soda0289]

@quantuminformation This is a known issue. We are working on making eslint traverse type annotations and decorators to allow for the rule no-unused-vars to work as expected.

Check out this issue for progress:
eslint/eslint#7129

I have created a patch to get this to work with eslint-scope.
eslint/eslint-scope#26

We currently use a rule in typescript eslint plugin that can mark decorators as used but not type annotations. Either way once eslint can traverse typeannotions we can update the eslint plugin to mark them as used or use new escope logic.

[quantuminformation]

thanks guys

[sompylasar]

@soda0289 RE: #179 (comment)

This eslint/eslint#7129 has been merged in eslint/eslint#8365

This eslint/eslint-scope#26 has been closed in favor of creating plugins/hooks.

What's the status of this? no-undef is really annoying on interfaces. Thank you!

[soda0289]

@sompylasar The issue of traversing type annotations or decorators will not fix the problem with no-undef or no-unused-vars. The problem lies in eslint-scope and not eslint or estraverse. My original assumptions about how eslint traversed nodes was incorrect.

For this to be fixed we need eslint-scope to understand typescript nodes that are produced by this parser's AST. The babel-eslint parser currently monkey patches eslint-scope to achieve this by overriding methods in the scopers referencer class. This solution is not ideal and I have suggested allowing the parser to pass its own set of VisitorKeys and eslint-scope node listeners or plugins.

The first stage of passing VisitorKeys to eslint-scope and estraverse will allow us to only traverse nodes that we know are handled correctly by eslint and eslint-scope. This will allow for no-undef to not throw false positives. This PR is waiting for TSC approval and review. eslint/eslint#8582

The second stage of fixing eslint-scope is a little more complicated. One solution is to add hooks into visitor functions and allow for the parser to override node listeners. eslint/eslint-scope#30. This is not perfect but will allow us to replace the monkey patch used in babel-eslint and fix issues with no-undef and no-unused-vars. A better solution would be to allow for multiple node listeners to exists and for plugins to be created for things like decorators, type annotations, and type parameters. This needs more thought and would require a bigger refactor of the eslint-scope code base.

There has been some other ideas of allowing parsers to supply their own scope anaylsis tool but this should all be discussed in the next TSC meeting and hopefully a more concrete plan will come forward.

[sompylasar]

@soda0289 Thank you for the detailed explanation! The unfortunate thing here is this will likely happen so far in the future, even though so many people are willing to help out... and eslint@4 will have to be adapted by the most popular rule template and every of its dependencies.

[soda0289]

Yes it will take time for plugins to update but ESLint v4 brings many fixes and features to this parser besides the possibility of better scope analysis. We could look into creating a monkey patch now for eslint v3 support but it only effects two rules and both of those rules can be enforced by the typescript compiler itself.

Another thing to keep in mind is that babel is working towards typescript support as well. Meaning it might be possible to use babel-eslint with typescript and eslint v3 depending on how things work out.

[sompylasar]

We could look into creating a monkey patch now for eslint v3 support but it only effects two rules and both of those rules can be enforced by the typescript compiler itself.

That would be great actually. I'm currently trying to introduce TypeScript into JavaScript project while not transforming the whole project to TypeScript, so I need that support from ESLint, not from TypeScript.

Another thing to keep in mind is that babel is working towards typescript support as well. Meaning it might be possible to use babel-eslint with typescript and eslint v3 depending on how things work out.

And I've succeeded in doing this right now (versus "might be possible") as I see immediate benefits of using TypeScript on the large-scale project I'm working on, so there is no time to wait, we have to push the boundaries.

I'd rather made tweaks to ESLint config ruleset based on the file type it's applied for, but that is not possible because ESLint does not provide the filePath to the config, assuming the config should be static and one for all, so the only thing I can do is to alter the parser, but not the rules.

// .eslintrc.js
  "parser": "./webpack/parserForEslint.js",

// ./webpack/parserForEslint.js
module.exports = {
  parse: function (text, parserOptions) {
    if (/\.tsx?$/.test(parserOptions.filePath)) {
      return require('typescript-eslint-parser').parse(text, parserOptions);
    }
    return require('babel-eslint').parse(text, parserOptions);
  },
};

[soda0289]

I'm not convinced we should add a monkey patch just for two rules but if no other solution can be reached we might be forced into it.

If you are looking for a better solution for using different parsers/rules for different files work is being done in eslint to make this happen.
eslint/eslint#8081
eslint/eslint#8543

We should probably start a new issue if you want to discuss this further.

[sompylasar]

And we've got the negative response from airbnb, they're not going to upgrade to eslint@4 in observable future. So if we really want to enable JavaScript people moving to TypeScript to use eslint (this repo's purpose), a hacky patch now is better than nothing now and something better in the distant future.

[sompylasar]

I ended up cutting TSModuleDeclaration nodes out of the AST to prevent no-undef from triggering within them:

module.exports = {
  parse: function (text, parserOptions) {
    if (/\.tsx?$/.test(parserOptions.filePath)) {
      const ast = require('typescript-eslint-parser').parse(text, parserOptions);

      // WORKAROUND(@sompylasar): Drop `declare module` to prevent its contents from triggering `no-undef`. https://github.com/eslint/typescript-eslint-parser/issues/77#issuecomment-293581001
      ast.body = ast.body.filter(function (node) {
        return (node.type !== 'TSModuleDeclaration');
      });

      return ast;
    }
    return require('babel-eslint').parse(text, parserOptions);
  },
};

[soda0289]

Closing this in favor of #77.