cartant / rxjs-tslint-rules

TSLint rules for RxJS
https://cartant.github.io/rxjs-tslint-rules/
MIT License
371 stars 22 forks source link

rxjs-no-compat not detecting rxjs-compat import for Observable.of #105

Closed DaanKeun closed 5 years ago

DaanKeun commented 5 years ago

Hi,

While attempting to adding the rxjs-no-compat rule to our list of RxJs rules we came across an issue where it was ignoring the rxjs-compat import for Observable.of

import 'rxjs-compat/add/observable/of';

Checking the current regex that detects invalid imports we noticed that currently the regex is not catering for observable namespace/directory in the import.

!/^['"]rxjs\/(ajax|operators|testing|webSocket)['"]$/.test(moduleSpecifier.getText())

cartant commented 5 years ago

I'm not sure what you mean. That regexp is for valid import locations and there is no valid observable location in v6 without rxjs-compat.

I'll have a look at the ignored import 'rxjs-compat/add/observable/of a little later.

DaanKeun commented 5 years ago

Ahh right, I see that now 🤦‍♂.

Then I'm not sure why it is ignoring it. Our current workaround is to add 'rxjs-compat' into the import-blacklist which is working for now so no rush.

cartant commented 5 years ago

I added a test to attempt a reproduction of the issue, but the test passes, so ... 🤷‍♂ See https://github.com/cartant/rxjs-tslint-rules/commit/4c679628967c18b0753ea1c9245709347803a4d0

DaanKeun commented 5 years ago

For the test should the import not be import 'rxjs-compat/add/observable/of';? Or am I misunderstanding how these tests work?

cartant commented 5 years ago

You've misunderstood. You use the old-style imports when using rxjs-compat. You should never have rxjs-compat in the import location. If you have a look in node_modules/rxjs you will find files in the places for the old import locations and those re-direct to rxjs-compat.

DaanKeun commented 5 years ago

Ahhh ok cool, thanks for taking the time looking into this 😄 and sorry for any wasted time

cartant commented 5 years ago

No worries.