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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong Error warnings for Unused patched operator in ./rxjs.imports.ts #8

Closed
Mischa1610 opened this issue Sep 15, 2017 · 21 comments
Closed

Comments

@Mischa1610
Copy link

Hello,

first I want to say thank you for your really good work here. 馃憤

So now to my problem:

I am using the rxjs-add rule like this:

"rxjs-add": {
    "options": [
        {
            "allowElsewhere": false,
            "allowUnused": false,
            "file": "./src/app/rxjs-operators.imports.ts"
        }
    ],
    "severity": "error"
},

So I use the approach to have all rxjs operators in on central file and nowhere else it is allowed to have them and don't have unused imports for the rxjs operators in the central rxjs operator import file.

That worked fine I just got the same error that was mentioned here: #5
I could fix that, but now I am getting linting errors for Unused patched operator although they are for sure used in my project.

I think this problem is related to the stuff that it is an angular project created with angular-cli and calling ng lint and that run's tslint three times (like you explained it):

  • first, with application files from src/;
  • then with the test files from src/;
  • and, finally, with files from e2e/.

So I think because not all application files are there for the second and third run I get the errors with the Unused patched operators.

For now, my only solution is to use "allowUnused": true.

Do you have an idea or hint how I could fix this problem?

@cartant
Copy link
Owner

cartant commented Sep 15, 2017

Have you seen #4? Are you definitely not using no-unused-variable?

@cartant
Copy link
Owner

cartant commented Sep 15, 2017

I think the only way to resolve it would be to include in the tests a file that contains an uncalled function that uses the unused operators. Unless the CLI's linting can be configured differently for each if the three runs, I cannot see an easy way around the problem.

Or you could aim for 100% test coverage. 馃榾

@Mischa1610
Copy link
Author

I saw #4 and it makes no difference either I use no-unused-variable or not.

Ok thank you for your information, I will try to dig deeper in the documentation/code of the CLI if I can configure the three run's (but don't think so).

100% test coverage with also testing the RxJS operators 馃榾 Now Way 馃榾

@Mischa1610
Copy link
Author

So my solution (for now) is:
I added a rxjs-operators.imports.spec.ts that has an exported class with a public static function where all imports (RxJS operators) are "called" (assigned to a variable).

Because it is a *.spec.ts file it is not in the "app" project source code.
So for the app the rule "allowUnused": false will still be linted/analyzed correctly.

For the .spec linting run no rxjs operators unused linting errors are reported (because all files with *.spec.ts are included).

To fix the rxjs operators unused linting errors also for the e2e run I just added to the tsconfig.e2e.json the include of the rxjs-operators.imports.spec.ts:

"include": [
    "**/*rxjs*imports.spec.ts"
]

If anyone reads this and knows a better solution I would be happy to hear it 馃榾

@cartant
Copy link
Owner

cartant commented Sep 15, 2017

I'm contemplating whether or not to implement something to better manage this - as using @angular/cli is commonplace.

I can see how the e2e tests will always be a problem, but how is it that you have application files that use RxJS operators that are not present in the second run of TSLint? That is, why are they not included when linting the test files from src/? Should they not at least be depended upon by a component or service that is imported into a spec?

@Mischa1610
Copy link
Author

Sorry, I am not sure if I got your question right.

But in the "second" run, the .spec run, only the *.spec.ts files are included (or if I import components or services that I want to test or to mock they are also included) from my app.
So when your rxjs-linting run's over my rxjs-operators.imports.spec.ts it sees all the RxJS operator imports from my "app" and then it run's over all files in the .spec run and there are for sure not all RxJS operator's used in the *.spec.ts file (test files). Same thing kinda for the e2e run but with *.e2e-spec.ts files normally. (I hope I explained that right and I hope I didn't get something wrong what is happening exactly in all the runs)

@cartant
Copy link
Owner

cartant commented Sep 15, 2017

Thanks. That makes sense. I'll have a dig around for the actual TSLint configurations, but you are surely correct. The application files will be loaded, by TypeScript, to build the program, but that doesn't mean they will be checked by TSLint. I'll give it some thought.

@Mischa1610
Copy link
Author

Currently, I would be happy if the rxjs-operators linting would just run for the "app" linting (the first run?).

Or if it is possible to configure something that it runs with "allowUnused": false in the "app" linting run (first run) and with "allowUnused": true in the second and third run 馃榾

Btw. thanks a lot for the really fast replies.

@Mischa1610
Copy link
Author

If you need a pretty easy project setup to try and play around with it, I could prepare something, or you just try it with a new created angular project with @angular/cli.

@cartant
Copy link
Owner

cartant commented Sep 16, 2017

I've had a quick look at this and I've added some notes to the README.md.

When using the file option, all files in the program - not just the specs - are walked to determine what is unused. So if unused errors are effected, there must be operators in files that are not imported into tests. The simplest way to solve the problem would be to import them into a test - even if they are not actually tested. Or, they could be added to src/tsconfig.spec.json - but that might be more tedious to maintain.

Closing this, but feel free to re-open if there are further problems. Happy to continue the conversation without re-opening, too.

@Mischa1610
Copy link
Author

Thank you a lot.
Your idea and description regarding @angular/cli in the README.md helped a lot and I now do it this way. 馃榾

Just a short information to the e2e/tslint.json, this file needs to be set in the .angular-cli.json otherwise the project tslint.json will be used:

...
"lint": [
    ...,
    {
        "project": "e2e/tsconfig.e2e.json",
        "tslintConfig": "e2e/tslint.json"
    }
],
...

@cartant
Copy link
Owner

cartant commented Sep 18, 2017

It's possible that it depends upon the version of @angular/cli that you are using. I'm using 1.4.2 and all I need to do is create a tslint.json file in the e2e directory with this content:

{
  "extends": ["../tslint.json"],
  "rules": {
    "rxjs-add": { "severity": "off" }
  }
}

The CLI's lint task should look for tslint.json files relative to the file that's being linted. You can see this in the source.

@Mischa1610
Copy link
Author

Oh cool.
Yep I currently using an older @angular/cli version. Good to know. Ty.

@chriswhite199
Copy link

@carcant - regarding your comment #8 (comment)

The simplest way to solve the problem would be to import them into a test - even if they are not actually tested

I've got this issue where defaultIfEmpty and reduce operators are being flagged as unused, even though it is used in a main source file, and the source file is being referenced by a .spec.ts file.

Interestingly it appears that chaining operators & reduce might be to blame, as if if separate my statement into multiple statements it doesn't throw up the unused error:

// This give unused patched operator
// buildTreeNodes returns Observable<TreeNode>
const obs: Observable<TreeNode[]> = this.buildTreeNodes().reduce(..).defaultIfEmpty([]);

// where as if you refactor as the following, it no longer flags up reduce 
// and defaultIfEmpty as being unused:
const obs: Observable<TreeNode> = this.buildTreeNodes();
obs.reduce(..).defaultIfEmpty([]);

@cartant
Copy link
Owner

cartant commented Nov 8, 2017

@chriswhite199 It's not clear from you comment whether or not the linting works as expected on the main source file alone. If it, too, fails, then it's a separate issue to this one.

If it does not, I suspect it's related to either:

  • the return type of buildTreeNodes causing a problem similar to this gotcha; or
  • you have TSLint's no-unused-variable rule enabled (either explicitly or by default).

The no-unused-variable rule had (and likely still has) some problems. See #4.

@chriswhite199
Copy link

Sorry for being unclear:

  • no-unused-variable is explicitly turned off - so it's not that
  • buildTreeNodes doesn't use Observable.create (it's a chain of Observable.range.map.concatMap.take.map) and the method explicitly defines its return type as Observable<TreeNode>

I guess the long and the short of it is it must be something similar to the gotcha as splitting over two lines means that the TypeChecker finds an Observable after the first line (as the const is explicitly typed).

@cartant
Copy link
Owner

cartant commented Nov 9, 2017

It's still not clear whether or not it's related to this issue. This issue is specific to the Angular CLI's multi-pass approach to linting. If splitting it into two lines completely solves your problem, it's a separate issue to this one. In which case, it would be preferable to open another issue.

@kamok
Copy link

kamok commented Dec 19, 2017

Can you please open this Issue?

I have the following in my tslint.json

...
"no-unused-variable": false,
...
  "rules": {
    "rxjs-add": {
      "options": [
        {
          "allowElsewhere": false,
          "allowUnused": false,
          "file": "./src/rxjs.imports.ts"
        }
      ],
      "severity": "error"
    },
    "rxjs-no-wholesale": {
      "severity": "error"
    },

Still getting
Unused patched operator in ./src/rxjs.imports.ts: methodName for everything I imported.

@cartant
Copy link
Owner

cartant commented Dec 19, 2017

@kamok Please refer to the README and the repo's example Angular CLI configuration. Note that the commit that adds the rxjs-tslint-rules config is here.

If none of that helps, please open a new issue, with specifics regarding your versions and configuration, etc. Particularly, if said configuration differs from the example.

@kamok
Copy link

kamok commented Dec 19, 2017

It turned out to be the unused variable stuff in tsconfig. I didn't know they were there, as well as tslint.json. Thanks...

@cartant
Copy link
Owner

cartant commented Dec 19, 2017

@kamok No worries. I appreciate your reading through the issues to find one related to your problem, but I think I speak for most maintainers in recommending that instead of commenting on a closed issue, a new issue should be opened with references to any relevant/related issues. As you can see from your resolution, it was not the same issue. Anyway, glad to hear that you have it working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants