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

RxJS 6: switchMap not unsubscribing depending on import of Observable #4363

Closed
bcmedeiros opened this issue Nov 20, 2018 · 10 comments
Closed

Comments

@bcmedeiros
Copy link

bcmedeiros commented Nov 20, 2018

Bug Report

I can still observe the same problem reported in #3306, but using RxJS 6, contrary to what is stated in #3306 (comment).

As #3306 is blocked for new comments, I'm reporting a new one to see if there is any action that can be taken.

Current Behavior
I have a typescript file creating an observable (merge) based on a few other observables, nothing out of the ordinary:

const merged$ = merge(
  a$.pipe(tap(() => console.log("side effect!"))),
  b$,
  c$
).pipe(
  ...
)
return merged$

For some weird reason, the rxjs-related imported were as follows:

import { Observable } from 'rxjs/internal/Observable'
import { merge, Subject } from 'rxjs/index'
import { Subscription } from 'rxjs/internal/Subscription'

This returned observable was then handled to a component, which would switchMap() a lot of times over the same instance of the returned observable. With the imports stated above, though, switchMap() was not unsubscribing properly, so on every switchMap()'s "switch", the previous subscribed observable (merged$) was still producing side effects, even after it was supposed to be unsubscribed, producing n+1 side effects for each a$'s emission after each "switch".

Expected behavior
switchMap() should properly unsubscribe the previous observable before subscribing to the next one. After changing the imports (see below), everything started working as expected.

Environment

  • Runtime: e.g. Node v10.4.0, Chrome v70.0.3538.77
  • RxJS version: 6.3.3

Possible Solution
Use the official import way stated in the docs:

import { merge, Observable, Subject, Subscription } from 'rxjs'
@kwonoj
Copy link
Member

kwonoj commented Nov 20, 2018

first, it may need reproducible code. Moreover, importing from internal is already red flag: that is not supported in any way, so we won't likely able to suggest workaround based on current import. What makes imports from unsupported internal scope and reason can't to properly imports?

@bcmedeiros
Copy link
Author

bcmedeiros commented Nov 20, 2018

What makes imports from unsupported internal scope and reason can't to properly imports?

Probably some buggy IDE behaviour caused the wrong imports, and I'm 100% with you that the wrong imports are the cause of the issue. After 1 and 1/2 day to realise that the imports were the cause of the issue, I'm able to fix my app changing the imports. I'm already doing that, actually.

I raised the issue here so we can find a way to prevent more people to fall into that, as it is pretty hard and time-consuming to debug this kind of issues.

importing from internal is already red flag: that is not supported in any way

Why is it possible at all, then? Sorry if it is a dumb question, but if it's possible to forbid this somehow, that would definitely avoid the problem.

@kwonoj
Copy link
Member

kwonoj commented Nov 20, 2018

Sorry if it is a dumb question, but if it's possible to forbid that somehow

We wish to limit it as internal scope, but typescript / javascript both doesn't have concept of internal scope module so exports are all public. Only way we could make it was naming under internal namespace (/internal).

@bcmedeiros
Copy link
Author

Well, I'm in favour of that, but I'm still a beginner in the RxJS world, so I'm not sure on how to help.

@cartant
Copy link
Collaborator

cartant commented Nov 20, 2018

Related to #4230 - to this comment, at least.

I'd really like to see a repro of the importing-from-internal-prevents-unsubscription problem.

@kareljuricka
Copy link

kareljuricka commented Nov 20, 2018

Confirming this issue. Last two days I spend on searching memory leaks in app.
I'm using combineLatest followed by map operators and after destroying of components, memory stayed uncleaned. When I was totally crazy about reasons (tried everything to unsubscribe), I found this new issue and it saved my ass.

My case:

changed:
import { combineLatest } from 'rxjs/internal/observable/combineLatest';
to:
import { combineLatest } from 'rxjs';

No I'm walking through system and fixing all wrong imports.
Those imports where automade by IDE webstorm. I knew about them but i ignored them before (didn't want to spend time with rewriting imports manually)

@cartant
Copy link
Collaborator

cartant commented Nov 20, 2018

@kareljuricka If you have confirmed it, please provide a minimal reproduction.

@bcmedeiros
Copy link
Author

bcmedeiros commented Nov 20, 2018

@cartant I can try to reproduce as well, I just didn't do that beforehand because it's going to be very hard and time-consuming, and it seems using internal imports is not supported anyway, so I'm wondering if it's worth doing (we might need to go as far as having the internal imports in a dynamically loaded angular module, for example, and I don't know how to replicate that in a minimal reproducible test).

@cartant
Copy link
Collaborator

cartant commented Nov 20, 2018

@brunojcm One thing you could do is provide the code that shows the internal import being used. You mention in the issue some internal imports, but you don't show them being used.

@benlesh
Copy link
Member

benlesh commented Jan 15, 2019

This appears to be resolved as "bad imports". Closing.

@benlesh benlesh closed this as completed Jan 15, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Feb 15, 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

No branches or pull requests

5 participants