Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autofix lightbulb not shown in TypeScript #609

Open
jacobrask opened this issue Jan 30, 2019 · 13 comments

Comments

Projects
None yet
7 participants
@jacobrask
Copy link

commented Jan 30, 2019

The fixes lightbulb with suppress options does not show up in TypeScript files. If I manually change the language mode of a file to JavaScript, it shows up, and then disappears again if I change it back to TypeScript. The error shows as expected and I have the following settings enabled:

    "eslint.validate": [
        "javascript",
        "javascriptreact",
        "typescript",
        "typescriptreact"
    ],
@dbaeumer

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

Fixes are only automatically shown for JS right now. You can enable for TS using the following setting:

    "eslint.validate": [
        "javascript",
        "javascriptreact",
        { "language": "typescript", "autoFix": true }
        { "language": "typescriptreact", "autoFix": true }
    ],

Keeping the issue open to consider if we should auto enable TS.

@dbaeumer dbaeumer added this to the On Deck milestone Feb 5, 2019

@svipas

This comment has been minimized.

Copy link

commented Feb 11, 2019

@dbaeumer I vote for an automatic validation and auto fix enabling for TypeScript files because of this https://github.com/typescript-eslint/typescript-eslint. I'm currently using it in VS Code and it works perfectly. I even think TSLint after some time will be abandoned... Even TypeScript team will use ESLint for their own repo, you can read it here: microsoft/TypeScript#29288 and here https://eslint.org/blog/2019/01/future-typescript-eslint

@dbaeumer

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

The only problem we need to keep in mind is that a misbehaving plugin might caused bad fixes in the code. This is why this is currently a opt in.

PR welcome to make this a opt-out.

@David-Else

This comment has been minimized.

Copy link

commented Mar 13, 2019

https://github.com/typescript-eslint/typescript-eslint seems to be working great, but I had to search through bug reports to find the above config info to make it work in VS Code... very bad user experience! Please enable it by default.

@dbaeumer

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

I quickly looked into this and it is a little bit more tricky. The problem is that the extension currently doesn't know if the eslint npm module is configured in a way that it will be able to validate TS files. If I enable this by default and eslint is not configured correctly I will validate TS files with a JS validator which has bad effects. See also #642

If anyone knows about some eslint API to figure this I would appreciate it. See also https://github.com/eslint/eslint/blob/master/docs/developer-guide/nodejs-api.md

@jacobrask jacobrask changed the title Fixes lightbulb not shown in TypeScript Autofix lightbulb not shown in TypeScript Mar 19, 2019

@saranshkataria

This comment has been minimized.

Copy link

commented Mar 20, 2019

Can we not read the eslintrc file to see if a typescript plugin is installed?

@agwells

This comment has been minimized.

Copy link

commented Mar 20, 2019

Could you use CLIEngine.getConfigForFile() to check for the presence of the @typescript-eslint plugin? https://github.com/eslint/eslint/blob/master/docs/developer-guide/nodejs-api.md#clienginegetconfigforfile

@dbaeumer

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

@saranshkataria I would like to avoid reading the eslintrc file myself for the following reasons:

  • can exist in different formats js, json, yml, ...
  • if its format evolve I need to catch up.

I so far tried hard to stay away from this.

@agwells nice idea CLIEngine.getConfigForFile(). Would still require understanding the return JSON strcuture but at least it doesn't require file looksups and interpretation.

@agwells

This comment has been minimized.

Copy link

commented Mar 21, 2019

I'm not sure that it is really necessary to make sure the user has their ESlint configured correctly, so long as:

  1. typescript and typscriptreact are not included in eslint.validate by default
  2. And eslint.autoFixOnSave is false by default

Then, if the user enables all that, they've taken a deliberate step and can assume the risk themselves.

For that matter, the risk of inappropriate fixes being applied to a TypeScript file seems fairly small, because ESLint itself, if it's not configured for TypeScript, is most likely to mark a TypeScript file as full of unfixable parsing errors. :)

@dbaeumer

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

@agwells 1. and 2. are actually the case but the requests ask to make this a better out of the box expierence. So include typescript and typscriptreact in the eslint.validate and even enable autofix. But this makes only sense when the linter is configured to lint TS files.

@ExE-Boss

This comment has been minimized.

Copy link

commented Apr 7, 2019

I think we should use CLIEngine.getConfigForFile(…) to determine whether ESLint is correctly configured to be able to lint TypeScript files, and if it’s not, we would give the user a suggestion to install the @typescript-eslint plugin.

@agwells

This comment has been minimized.

Copy link

commented Apr 7, 2019

I'm not sure that it is really necessary to make sure the user has their ESlint configured correctly, so long as:

1. `typescript` and `typscriptreact` are not included in `eslint.validate` by default

2. And `eslint.autoFixOnSave` is `false` by default
  1. and 2. are actually the case but the requests ask to make this a better out of the box expierence. So include typescript and typscriptreact in the eslint.validate and even enable autofix. But this makes only sense when the linter is configured to lint TS files.

To me these settings are fine as-is. As I see it, this excellent plugin already has all the functionality it needs, it's just that this one config setting is confusing. :)

Specifically, as a user of the plugin, I expected that if I enter this in my settings...

    "eslint.validate": [
        "javascript",
        "javascriptreact",
        "typescript",
        "typescriptreact"
    ],

... that it would result in TypeScript and TSX getting the same level of functionality as JavaScript and JSX.

The ability for the plugin to auto-detect the TS configuration etc, would be nice. But I would be very happy with just this small change, of only requiring "typescript" in the settings instead of { "language": "typescript", "autoFix": true }

@dbaeumer

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

But I would be very happy with just this small change, of only requiring "typescript" in the settings instead of { "language": "typescript", "autoFix": true }

This is actually easy and I can add this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.