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

ng lint is linting node_modules since Beta 28.3 #4350

Closed
maxime1992 opened this issue Feb 2, 2017 · 27 comments · Fixed by #4437
Closed

ng lint is linting node_modules since Beta 28.3 #4350

maxime1992 opened this issue Feb 2, 2017 · 27 comments · Fixed by #4437

Comments

@maxime1992
Copy link
Contributor

maxime1992 commented Feb 2, 2017

Please provide us with the following information:

OS?

Windows 7, 8 or 10. Linux (which distribution). Mac OSX (Yosemite? El Capitan?)
Ubuntu 16.10 x64

Versions.

Please run ng --version. If there's nothing outputted, please run in a Terminal: node --version and paste the result here:

angular-cli: 1.0.0-beta.28.3
node: 7.4.0
os: linux x64
@angular/common: 2.4.5
@angular/compiler: 2.4.5
@angular/core: 2.4.5
@angular/flex-layout: 2.0.0-beta.4
@angular/forms: 2.4.5
@angular/http: 2.4.5
@angular/material: 2.0.0-beta.1
@angular/platform-browser: 2.4.5
@angular/platform-browser-dynamic: 2.4.5
@angular/router: 3.4.5
@angular/compiler-cli: 2.4.5

Repro steps.

Was this an app that wasn't created using the CLI? What change did you do on your code? etc.

Upgraded to angular-cli beta.28.3 and running ng lint gives me a high number of lint error that can be found in ... node_modules.

  • I tried to repro with fresh project but couldn't
  • Tried to compare all related files between fresh and existing repo : Didn't notice anything that might have that kind of impact
  • Tried to remove my yarn.lock and reinstall with fresh deps : Didn't change

Mention any other details that might be useful.

When I upgraded from beta 26 to beta 28.3, the lint property was added into angular-cli.json :
image

@victornoel found something interesting in the code tho :
https://github.com/angular/angular-cli/blob/master/packages/%40angular/cli/tasks/lint.ts#L32

image

We get config.project and store it into program .
Then we get the files from Linter.getFileNames(program); and not from config.files.

Thus, this part of our conf is never used :
image

Why is it working well in a fresh project, I have no idea ...

@delasteve
Copy link
Contributor

I will take a look at this today.

@delasteve
Copy link
Contributor

@maxime1992 - I've been looking into this and have been creating many different files. Unfortunately, I haven't come across an issue. I will address the files vs project property in another PR (basically it will use project by default unless it sees files). Another PR will introduce the exclude option.

Would anyone reading this that has this issue be able to paste your tsconfig.json for me or create an example project where it has this issue?

@maxime1992
Copy link
Contributor Author

@delasteve the only thing I can do to help is to give you my repo as I couldn't repro in a smaller project ...

git clone https://gitlab.com/victornoel/petals-cockpit.git
cd petals-cockpit/frontend
yarn
# or npm install
ng lint

If you want to take a look into the commit I made to upgrade to beta 28.3, here it is :
https://gitlab.com/victornoel/petals-cockpit/commit/e669b31da03edf0c4e3bbd4bd94de85632efb435

@delasteve
Copy link
Contributor

@maxime1992 thanks! I'll take a look at it in a bit.

@sguiheux
Copy link

sguiheux commented Feb 3, 2017

same problem on beta.30

@sguiheux
Copy link

sguiheux commented Feb 3, 2017

Trying to debug lint.js on angular-cli project:
In Linter.js: parsed.fileNames contains the good files
But program.getSourceFiles() (Linter.js.getFileNames) contains node_modules files

@sguiheux
Copy link

sguiheux commented Feb 3, 2017

That 's normal : https://github.com/palantir/tslint/blob/master/src/linter.ts#L79, the getSourceFiles() method get also all referenced files.

@sguiheux
Copy link

sguiheux commented Feb 3, 2017

Maybe angular-cli should have a parameter to use

  • Linter.GetFileNames for people who want to check all files,
  • program.getRootFileNames that do not contains files from import.

@delasteve
Copy link
Contributor

delasteve commented Feb 5, 2017

@maxime1992, @sguiheux: With #4437, it will be default use the tsconfig.json file if the files property isn't set. I also included a new exclude property that will take glob(s).

I checked against your repo, @maxime1992, and it excluded the node_modules folder.

The angular-cli.json generated when you ng new will be sufficient enough to always exclude node_modules, but if you chose to remove files (an optional property) and still wanted to exclude node_modules, you would need to do this:

"lint": [
  {
    "project": "src/tsconfig.json",
    "exclude": "**/node_modules/**/*"
  }
],

delasteve added a commit to delasteve/angular-cli that referenced this issue Feb 5, 2017
@maxime1992
Copy link
Contributor Author

maxime1992 commented Feb 5, 2017

@delasteve Wow thanks ! 🎉

Gonna try that ASAP this week ! 😄

@Almar
Copy link

Almar commented Feb 5, 2017

@delasteve, thank you for your work. I don't understand your last message though. Have you figured out why node_modules was included when linting in @maxime1992 project? The files property was set so he would not have needed the excludeoption, correct?
I'm having the same problem and I don't understand what the difference is between a newly created project and an upgraded one.

@delasteve
Copy link
Contributor

@Almar, from what I can tell, this is what is happening... (tl;dr provided at end)

node_modules was included because it included .ts files. The library should not have included the .ts files in the published npm module. It doesn't happen with a fresh project because all the dependencies are correctly published (as in they only published the transpiled output, .js and .d.ts).

The reason it was looking into the node_modules folder even with files being set was because I never had passed the files property in to the linter. I had got the list of files to Linter after passing in a Program.

What I was doing:

const program = Linter.createProgram(config.project);
const files: string[] = Linter.getFileNames(program);

This returns all the files seeing by the typescript compiler. Since, as mentioned, the .ts files were published in the library, the Program picked them up as not a library, but as source files, and, thus, TSLint would lint them.

With my changes, if files (optional, string or string[]) is passed, it will use the glob(s) to get the files. If no files are passed, it will use the tsconfig.json from the project (required, string) property. I added an exclude (optional, string or string[]) property so that you can use that to exclude the node_modules folder, among others.

You can read more about the project property here (as I tried to keep the names the same from the tslint cli): https://github.com/palantir/tslint#cli-1

tl;dr: It works how you expect it to work going forward. If you followed the BREAKING CHANGE or ng update diff, you should be good to go when the PR gets merged.

@Almar
Copy link

Almar commented Feb 6, 2017

@delasteve, thank you for your comprehensive answer.

@Nasicus
Copy link

Nasicus commented Feb 7, 2017

I don't get it what the workaround for beta30 is... I tried the following in my angular-cli.json and none of it works:

This is what I had after the update:

  "lint": [
    {
      "files": "src/**/*.ts",
      "project": "src/tsconfig.json",
    },

This is what I tried:

  "lint": [
    {
      "files": "src/**/*.ts",
      "project": "src/tsconfig.json",
      "exclude": "**/node_modules/**/*"
    },

and

  "lint": [
    {
      "project": "src/tsconfig.json",
      "exclude": "**/node_modules/**/*"
    },

As mentioned none of it works for me - it always includes stuff from node_modules :(

@victornoel
Copy link

@Nasicus it's normal, you have to wait for a new release of angular-cli that contains the fixed discussed here :)
for now you can execute tslint by hand (like it was before in the package.json).

@delasteve
Copy link
Contributor

@Almar, @maxime1992, @Nasicus, @sguiheux, @victornoel: beta.31 was released last night. Please, could you update and tell me if you're still seeing any issues? Thanks!

@victornoel
Copy link

@delasteve as soon as we can find a solution to #4584 :)

@victornoel
Copy link

@delasteve seems to work good with the default files parameter in the angular-cli.json file :)

@delasteve
Copy link
Contributor

👍 Thanks @victornoel

@sguiheux
Copy link

@delasteve yes fixed too. Thank you

@anilkhichar
Copy link

The only working solution for me was to add files against all linting files

.angular-cli.json

"lint": [{
      "files": "src/**/*.ts",
      "project": "src/tsconfig.app.json"
    },
    {
      "files": "src/**/*.ts",
      "project": "src/tsconfig.spec.json"
    },
    {
      "files": "src/**/*.ts",
      "project": "e2e/tsconfig.e2e.json"
    }
  ]

@kvanlaan
Copy link

kvanlaan commented Jun 12, 2017

Hi @khichar-anil, the above does not work if you actually import from the node_module.

@maxime1992
Copy link
Contributor Author

Umh I might be wrong @kvanlaan but I think it's the whole point here.

@kvanlaan
Copy link

kvanlaan commented Jun 13, 2017

@maxime1992 The point of this thread is to find a way to disable linting for node_modules. If you use "files": "src/**/*.ts" method. It appears to work at first. But if you import a module from the node_module, then ng lint will lint the node_module.

@victornoel
Copy link

@kvanlaan the solution is to use exclude like so:

  "lint": [
    {
      "project": "src/tsconfig.app.json",
      "exclude": "**/node_modules/**"
    },
    {
      "project": "src/tsconfig.spec.json",
      "exclude": "**/node_modules/**"
    },
    {
      "project": "e2e/tsconfig.e2e.json",
      "exclude": "**/node_modules/**"
    }
  ],

@Splaktar
Copy link
Member

Splaktar commented Aug 2, 2017

If you want to add multiple exclusion rules, you can do it like this:

  "lint": [
    {
      "project": "src/tsconfig.app.json",
      "exclude": [
       "**/node_modules/**/*",
        "**/app/shared/api/**/*"
      ]
    },
    {
      "project": "src/tsconfig.spec.json",
      "exclude": [
       "**/node_modules/**/*",
        "**/app/shared/api/**/*"
      ]
    },
    {
      "project": "e2e/tsconfig.e2e.json",
      "exclude": [
       "**/node_modules/**/*",
        "**/app/shared/api/**/*"
      ]
    }
  ],

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants