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

No detection of inherited observables like @ngrx/store/Store #37

Closed
seckardt opened this issue Feb 19, 2018 · 8 comments
Closed

No detection of inherited observables like @ngrx/store/Store #37

seckardt opened this issue Feb 19, 2018 · 8 comments

Comments

@seckardt
Copy link

In case of inherited Observable logic the linting currently doesn't detect issues. Such cases are @ngrx/store/Store or @ngrx/store/Action.

For instance the following code would pass the linting, though a rxjs-no-operator rule is present:

...
import { Action, Store } from '@ngrx/store';
import { Actions, Effect } from '@ngrx/effects';

@Injectable()
export class PageEffects {
  constructor(
    private actions$: Actions,
    private store: Store<...>
  ) {}

@Effect()
myEffect$: Observable<Action> = this.actions$
    .ofType(...)
    .withLatestFrom(this.store.select(...).pluck('myPath'))
    .map(...);
@cartant
Copy link
Owner

cartant commented Feb 19, 2018

The rxjs-no-operator doesn't use type information. It looks only at the import statements.

Did you mean to use rxjs-no-patched?

@seckardt
Copy link
Author

Sorry, got the operators wrong. The whole list I'm actually using is:

{
  ...
  "rxjs-no-add": true,
  "rxjs-no-deep-operators": true,
  "rxjs-no-operator": true,
  "rxjs-no-patched": true,
  "rxjs-no-wholesale": true,
   ...
}

@cartant
Copy link
Owner

cartant commented Feb 19, 2018

Do you have a public repo that effects the behaviour? If not, the issue needs to include the version information for all relevant components.

Also, have you established that it is using the configuration you think it is? E.g. does adding an import that violates the rxjs-no-wholesale rule effect the error you'd expect?

@seckardt
Copy link
Author

seckardt commented Feb 19, 2018

Unfortunately it's in a private repo. Detections that directly refer to Observale or Subject perfectly work, it's just the inherited stuff that seems to cause issues. The relevant versions I'm currently using are:

{
  ...
  "@angular/cli": "1.6.8",
  "@angular/compiler": "5.2.5",
  "@angular/compiler-cli": "5.2.5",
  "@ngrx/effects": "5.1.0",
  "@ngrx/store": "5.1.0",
  "rxjs": "5.5.6",
  "rxjs-tslint-rules": "3.12.0",
  "tslint": "5.9.1",
  "typescript": "2.6.2",
  ...
}

@cartant
Copy link
Owner

cartant commented Feb 19, 2018

Okay. The problem is that to fix any bug, I need to reproduce it and I have the rules working with various projects that use Angular 4/NgRx 2 and Angular 5/NgRx 4. I've not yet upgraded to NgRx 5 - that could be the problem, but I'd be surprised.

A couple things to look at first:

  • If you (or some set of rules that you extend) use TSLint's no-unused-variable rule all bets are off. That rule breaks things. See Beware of TSLint's no-unused-variable rule #4. (If you remove all rules except for rxjs-no-patched, is the expected error still not effected?)
  • I've found a strange, upstream, OS-dependent bug that prevents rules from obtaining the required type information. Perhaps this could somehow be related? See Differing behaviour on Linux and Windows #32. Without my looking into the bug, I cannot say for sure.

The mechanism the rules use to determine the type is pretty basic. It just walks the type information looking for a base class named "Observable" - it just checks for the string in the type name. Apart from the above-mentioned bug, this has been working for some time.

If you are able to, it would be a huge help if you could fork this repo and modify the Angular CLI example contained within it. If you are able to reproduce the problem and point me at the fork, it would greatly help in my fixing any bugs.

@cartant
Copy link
Owner

cartant commented Feb 19, 2018

Also, any history of your using the rules would be valuable. Is this something that used to work for you? Has the bug accompanied your upgrading any components? Or are you only just starting to use the rules?

@seckardt
Copy link
Author

Thanks! I indeed use the no-unused-variable rule. Disabling it gives all the expected errors.

@cartant
Copy link
Owner

cartant commented Feb 19, 2018

Yeah. I really wish they'd either fix that rule or just can it. It causes so many problems.

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

2 participants