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

Not showing all errors when using ESLint processor on a file #922

Closed
JamesHenry opened this issue Mar 14, 2020 · 9 comments
Closed

Not showing all errors when using ESLint processor on a file #922

JamesHenry opened this issue Mar 14, 2020 · 9 comments
Labels
info-needed Issue requires more information from poster

Comments

@JamesHenry
Copy link

Hi @dbaeumer, thanks for all the fantastic work you do maintaining this extension.

For the first time I am making use of an ESLint processor (I am starting with that context because perhaps there are known issues with processors I have not come across before) in a plugin I am working on for ESLint to support Angular.

The project monorepo can be found here: https://github.com/angular-eslint/angular-eslint

The reason a processor is needed is that an Angular Component is written as a TypeScript file, but can contain an inline template definition (Angular-flavoured HTML within a string).


Issue repro

Using an integration test from the above project as our example, this file shows the inline template I mention and it has some intentional linting issues:

import { Component, OnInit, Output, EventEmitter } from '@angular/core';

@Component({
 selector: "app-example",
 template: `
   <input type="text" name="foo" ([ngModel])="foo">

   <app-item ([bar])="bar" ([item])="item" [(test)]="test"></app-item>
   <div [oneWay]="oneWay" (emitter)="emitter" ([twoWay])="twoWay"></div>
 `,
 styleUrls: ['./example.component.scss'],
 inputs: [],
 outputs: [],
 host: {}
})
export class ExampleComponent implements OnInit {

 @Output() onFoo = new EventEmitter();

 constructor() { }

 ngOnInit() {
 }

}

My ESLint processor and plugin are successfully working from the command line - I receive both the errors in the TS source code as well as the errors in the inline template reported to me using the Angular CLI builder which behind the scenes is using CLIEngine (source also available in that monorepo under /builder):

image

However, in VSCode I only see the issues highlighted within the TS source, not within the inline template:

image

Note that it is not an issue with HTML in general. because external (non-inline) templates as .html files work fine in VSCode and via the command line:

image

All you should need to do to reproduce is clone the repo and run yarn and then yarn integration-tests, and then open the integration test project at path ./packages/integration-tests/fixtures/angular-cli-workspace

@dbaeumer
Copy link
Member

Actually when I validate the file in the terminal using the same eslint version and working directory the ESLint plugin sees I don't get the extra errors eithers. Here is what I did:

  • project setup as described by you
  • cd into ./packages/integration-tests/fixtures/angular-cli-workspace
  • open vscode
  • the ESLint output channel tells me that eslint is loaded from [Info - 11:27:36 AM] ESLint library loaded from: /home/dirkb/Projects/mseng/VSCode/Playgrounds/bugs/angular-eslint/node_modules/eslint/lib/api.js
  • running ESlint in a terminal on angular-cli-workspace using ../../../../node_modules/.bin/eslint src/app/example.component.ts produces

capture

Could this be a setup/configuration problem on your end ?

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Mar 16, 2020
@JamesHenry
Copy link
Author

JamesHenry commented Mar 16, 2020

I’m not in front of a computer right now but did you include HTML files in your ext?

The processor extracts the template as an HTML file (implementation detail of processor lifecycle).

@dbaeumer
Copy link
Member

Not sure what you mean. All I noticed that for me following the instruction you gave me VS Code and the terminal produces the same result.

@JamesHenry
Copy link
Author

JamesHenry commented Mar 16, 2020

Back at a computer now. You are not comparing like for like because you are not providing ESLint with ext settings to include HTML, ESLint will not consider HTML by default.

Internally the pre part of the ESLint processor extracts the template part and creates a kind of fragment file which gets a name and appropriate extension (.html) so that it can be parsed and linted, and then the post part of the processor ties the location back to the original source file. (The end user is never aware this fragment file ever existed which is what I meant about it being an implementation detail of the processor lifecycle https://eslint.org/docs/developer-guide/working-with-plugins#processors-in-plugins).

Please see here for the behaviour with and without ext using your repro above:

image

As I included in the original issue description above, the vscode extension does seem to work fine when the root file is HTML, which is what led me to suggest it might be specific to this processor use-case.

Hopefully now the discrepancy is clear, let me know if you need any more info!

@dbaeumer
Copy link
Member

Have you tried to use the eslint.options setting to provide the --ext option to the CLIEngine. If you have sepecial command line parameter needs you need to pass them to the VS Code extension as well using the eslint.options setting.

@JamesHenry
Copy link
Author

I had previously set:

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

I have now also added:

"eslint.lintTask.options": "--ext .ts,.html"

...to match the above.

There is no change in behaviour after restarting VSCode, is it working for you via this or some other editor setting?

@dbaeumer
Copy link
Member

The setting you need to use is eslint.options. Something like

"eslint.options" = {
    "ext": ".ts,.html"
}

@JamesHenry
Copy link
Author

Ok so given it is being passed directly to CLIEngine, the setting needs to be:

"eslint.options": {
        "extensions": [".ts", ".html"]
    }

This works! That is great, thank you for helping find a solution.

I would say it is little counter intuitive to have "eslint.validate": [] set with HTML included but then have to set directly related data again in this way just for this processor use-case. I do now get the technical distinction, but doesn't the extension have all the information it needs to apply this automatically?

For example above I am now kind of telling the extension two different things -

  1. consider these files:
    "javascript",
    "javascriptreact",
    "typescript",
    "typescriptreact",
    "html"

  2. actually consider these files
    .ts
    .html

I think there will always be this risk of conflicting configs and subtle issues if this isn't incorporated into the extension directly.

Do you see what I mean?

@dbaeumer
Copy link
Member

The files types listed under (1) basically denote the files which the VS Code extension should sync to the server to be validated. It is like passing the file to validate when running in the terminal. In your special case there is a sibling file with a different extension that should be validated as well.

I would actually prefer to keep them different for now. I am pretty sure that if I make the suggested change other will complain.

I opened #929 so that the info doesn't get lost.

Closing the issue for now.

@vscodebot vscodebot bot locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
info-needed Issue requires more information from poster
Projects
None yet
Development

No branches or pull requests

2 participants