Rxjs: Observable from webpack library doesn't unsubscribe

[samal-rasmussen]

Bug Report

Current Behavior
The source observable doesn't unsubscribe when unsubscribing an observable that came from a library that returns rxjs observables.

The libray was bundled with Webpack.

Reproduction
https://github.com/samal84/rxjs-import-from-library-test

Environment

• Runtime: node 10.16.3, Chrome 77, Windows 10
• RxJS version: 6.5.3
• Webpack: 4.41.0

[BioPhoton]

Hi @samal84.

I run your repository to reproduce it. It's truly not working. I also changed you test observable to use Interval to even better demonstrate the problem.
PRs are also sent to your repo with the changes.

In your application part, I copied the same code that you compile to compare it. This works as expected.

So the problem lies in your compilation process and can IMHO not be fixed in the library.
I suggest looking for how rxjs is compiled and what the difference is.

If @kwonoj or @cartant agrees, I would suggest closing this one.

[kwonoj]

In example first rxjs is bundled in lib via webpack, and then another rxjs is bundled in application. This creates 2 sets of rxjs in application which doesn't passes instance check. Application should only have 1 set of observable.

[samal-rasmussen]

Thanks BioPhoton for the pull requests. I merged them.

I also added a passing test case without pipe or switchMap:

test$().pipe(
    take(1),
)
.subscribe(getObserver('compiled direct no pipe and switchMap'));

@kwonoj Does this mean that returning rxjs observable instances from libraries is not supported?

[kwonoj]

rxjs observable instances from libraries is not supported

no, library definitely can return observable as it needs, but in your application example you have 2 sets of rx (more than 1). That's problem need to be fixed.

[samal-rasmussen]

rxjs observable instances from libraries is not supported

no, library definitely can return observable as it needs, but in your application example you have 2 sets of rx (more than 1). That's problem need to be fixed.

The library in this case is using rxjs internally and then returning rxjs observables to callers. So it needs rxjs to be bundled along with it, right? Or else it cannot use rxjs internally.

The application is also using rxjs itself, so it also needs rxjs bundled with it for the same reason.

So this means that the lib and application get bundled separately, and get rxjs bundled into them separately.

How would we have only one rxjs when using rxjs inside both the lib and the app?

[samal-rasmussen]

Or I could try having the library bundle the whole of rxjs into it using npm bundledDependencies and have the applications that use the library import rxjs from the library:

https://docs.npmjs.com/files/package.json#bundleddependencies

I don't really want to even try this, as I don't want the applications to be dependent on the rxjs version that happens to be bundled in the library, and it seems like it would make the library bundle super heavy.

[cartant]

I think this could be a bug. I'll have a closer look at this tomorrow. Specifically, I think it might have something to do with this: https://github.com/ReactiveX/rxjs/blob/master/src/internal/symbol/rxSubscriber.ts#L4

[kwonoj]

@cartant it is not a bug. we do use instanceof checks as well after we remove symbol check https://github.com/ReactiveX/rxjs/pull/4182#issuecomment-424167624 which also does not gaurantee behavior acorss multiple instance.

from beginning of v5 we never supported multiple instances.

[cartant]

Nah. This is a bug for sure. I agree that the previous changes that related to different copies of RxJS were necessary because previous implementations were trying to be clever regarding trusted subscribers, but this problem is a little different. It should be possible to use observables from other, 'foreign' packages - including other versions or locations of RxJS. Basically, this problem is not cause by attempting to re-use a trusted Subscriber instance. It's caused by wrapping one in a SafeSubscriber.

I've added a failing test here:

https://github.com/cartant/rxjs/blob/4a347ee26feed258afb7fae5a8dd76aeba2e8385/spec/util/subscribeToObservable-spec.ts#L93-L108

And I'm thinking about the best way to solve it. The problem was introduced with a change to subscribeToResult that allowed it be be passed a specifically created subscription. And that subscription is added to the observable's destination - for teardown upon unsubscription. However, in the case of a 'foreign' observable, that subscription will be wrapped in a SafeSubscriber and because the wrapping SafeSubscriber is not added to the destination, it's not torn down upon explicit unsubscription.

Changing this https://github.com/ReactiveX/rxjs/blob/865b7d36c1fc59d065072e7026e959c3a0e01ce7/src/internal/operators/switchMap.ts#L131-L140

To something like this fixes it:

  private _innerSub(result: ObservableInput<R>, value: T, index: number) {
    const innerSubscription = this.innerSubscription;
    if (innerSubscription) {
      innerSubscription.unsubscribe();
    }
    const innerSubscriber = new InnerSubscriber(this, undefined, undefined);
    const destination = this.destination as Subscription;
    destination.add(innerSubscriber);
    this.innerSubscription = subscribeToResult(this, result, value, index, innerSubscriber);
    if (this.innerSubscription !== innerSubscriber) {
      destination.add(this.innerSubscription);
    }
  }

but I'm not sure whether or not that's the best solution. And that mechanism is used in a number of operators.

Still thinking about it. But this is a bug.

[kwonoj]

I still disagree: it was known, expected behavior to not support multiple instance of observable in single context, reasonnwhy there is plugin like webpack-rxjs-externals exists. What we gaurantee is observable will work in an application when it have single instance only. For other cases, though we could say it's improvement request it was never supported.

[cartant]

The problem is that that cannot be easily enforced with Node. My recollection is that the fixes that were made were made to avoid problems when there are multiple RxJS packages in node_modules.

In any case, this is an interop problem, as the subscription returned by a subscribe call to an interop observable is ignored! It just so happens that RxJS packages in different locations are considered to be interop these days.

Specifically, with the changes made in these PRs, returning interop observables in a flattening operators will leak like crazy unless this bug is fixed:

• https://github.com/ReactiveX/rxjs/pull/2479
• https://github.com/ReactiveX/rxjs/pull/4037

[es-repo]

I have an opposite issue: with subscription. A subject within an application subscribed to a subject created in my Webpack library does not receive values from it. Like subscription doesn't work. To workaround I have to pass Subject factory (() => new Subject()) as additional argument to functions of the library from the application. I think the reason of these bugs is the same.

[cartant]

I have an opposite issue: with subscription.

Probably warrants opening a separate issue then.

FWIW, commenting on an issue with regards to a separate issue is generally a time waster for maintainers because the comment doesn't fill out the template. Please open a new issue, fill out the template and reference this issue if you think it is related.

[samal-rasmussen]

A subject within an application subscribed to a subject created in my Webpack library does not receive values from it.

I have that situation set up and wokring in the reproduction github repo linked in the top comment. That repo is an as minimal setup of two separate webpack builds talking rxjs from one to the other as I could manage to set up, so maybe it is a good starting point for making a minimal reproduction of your particular issue.

[es-repo]

Please open a new issue,

Opened #5105. It's a somewhat different issue I thought initially.

[samal-rasmussen]

@cartant I just tried updating the linked reproduction repo to use rxjs 6.5.4 and I see the same behavior still. Doesn't seem like #5178 fixed this particular case.

[cartant]

@samal84 I can see the problem - well, I'm pretty sure I've found it, but I am rather tired. It's the same fix that's in the PR you linked. It needs to be applied in subscribeToObservable, too. I'll have another look at this tomorrow. Thanks.

[cartant]

@samal84 You didn't update both versions of RxJS. This is still using the version that has the bug: https://github.com/samal84/rxjs-import-from-library-test/blob/ae475509b9f9e4f5b8d00b6241282bab366f32e0/application/package.json#L8

If both use RxJS 6.5.4, it behaves as expected, AFAICT.

[samal-rasmussen]

Oh that's right. I can confirm it works. Thanks a lot @cartant!