ReactiveX / rxjs

A reactive programming library for JavaScript
https://rxjs.dev
Apache License 2.0
30.79k stars 3k forks source link

Observables cannot be passed between window contexts. #2628

Closed goldsam closed 6 years ago

goldsam commented 7 years ago

RxJS version: 5.4

Code to reproduce:

if (window.opener) {
  window.myObservable = window.opener.myObservable;
} else {
  window.myObservable = Rx.Observable.of(1, 2, 3);
  window.open(location.href);
}

window.myObservable
  .switchMap(function(x) {
    return Rx.Observable.of(x);
  })
  .subscribe(function(x) {
    console.log(x);
  });

Expected behavior: Both parent and child window should print 1, 2, 3 to the console.

Actual behavior: The child window throws an exception because myObservable from the parent window's context cannot be identified as an Observable in the child window's context.

Additional information: The issue is caused by a reliance on instanceof for determining if an object is an Observable which fails for objects passed between javascript contexts. Instead, a more robust solution would test for the shape of an Observable, perhaps using a type guard similar to isPromise

kwonoj commented 7 years ago

I'm feeling kind of similar to what bacon.js did. Checking properties in observable is not robust way (yes, even isPromise is same but promise is external dependency we can't control). My 5 cent is adding same description as bacon.js and let consumer should deliver instances between.

goldsam commented 7 years ago

At the end of the issue I referenced, bacon.js accepted a PR which eliminated the use of instanceof for testing Observable identity. After a quick browse at their HEAD, master still seems to have those changes incorporated.

Also, this project already acknowledges the interop point defined by the es7-observable spec, which I interpret as implying compatibility with external implementations of Observable. Currently, such compatibility is broken due to use of instanceof.

staltz commented 7 years ago

I agree that we should rely less on instanceof and use duck-typing instead. I know it's not robust, but JavaScript itself isn't robust.

mattpodwysocki commented 7 years ago

@goldsam @staltz I agree with the changes proposed here. We should be relying on the subscribe method here much as the Promise relies on then. The usage of instanceof is far from ideal here and shouldn't really be used.

hermanbanken commented 7 years ago

When instrumenting Rx for RxFiddle i've stumbled on this issue too. RxFiddle possibly uses a different instance of Rx than the instrumented target, which might be a small minified and tree-shaken module that contains a small subset of Rx. I've not really settled how to solve solve it. For now I just use only the types of Rx (import * as RxType from "rxjs") and make sure that the instrumentation module not actually uses RxType other than as a type. The I just take the provided Rx object or the globally available Rx object (window.Rx) to apply instanceof.

I agree with @goldsam however that Rx should interop with the es7 Observables. Just out of the box and by only looking at the contract (subscribe, next, error, complete).

staltz commented 7 years ago

We should be relying on the subscribe method here much as the Promise relies on then. The usage of instanceof is far from ideal here and shouldn't really be used.

Actually we should not rely on subscribe for duck typing. That's an ES Observable method, and there many other libraries that support the ES Observable interface, such as Most.js, xstream, Kefir. Under duck typing, all those streams from those libraries would be considered RxJS Observables. This would actually break real software for people using Cycle.js which allows both RxJS and xstream co-existing.

I'd argue for using a combination such as checking for lift plus subscribe, or _isScalar.

hermanbanken commented 7 years ago

The idea of Rx is to have a subscribe method on the Observable taking an Observer with a next, error and complete method. As long as you put in something that works with that we should be fine right?

In what cases does Rx use instanceof? Mostly in cases where we need to check whether we can use some optimization?

Why should Rx not work directly with Most, xstream, etc?

staltz commented 7 years ago

Why should Rx not work directly with Most, xstream, etc?

@hermanbanken through the ES Observable spec it indeed does work directly. The problem is when a window context receiving an Observable expects that Observable to be a real RxJS v5 Observable including things like the lift method. If that window context assumes that lift exists on the Observable, because ES Observable doesn't prescribe lift, then that window context will break if it receives an ES Observable from Most or xstream. In other words, RxJS Observable != ES Observable. You can pass ES Observables safely between window contexts, but you cannot pass RxJS Observables safely between window contexts, even if every Rx Observable is an ES Observable.

hermanbanken commented 7 years ago

I might not have been clear.

The problem is when a window context receiving an Observable expects that Observable to be a real RxJS v5 Observable

When does this occur? The original question gives this example:

window.myObservable
  .switchMap(function(x) {
    return Rx.Observable.of(x);
  })
  .subscribe(function(x) {
    console.log(x);
  });

here we see that it is switchMap receiving a foreign object.

including things like the lift method

The only thing that switchMap needs to verify is that Rx.Observable.of(x) has a subscribe method, is a Promise or is convertible to an Observable in some way. It does not need to have lift, it only needs subscribe.

I think the danger comes from use cases like this:

import map from "rxjs/operators/map"

let mappedOnce = window.parent.myObservable.map(x => x * x)
let mappedTwice = map.call(mappedOnce, x => x + 1)

That way two unrelated (by prototype) Observables are joined by creating one with composed selectors. Users of rxjs need to know how map is implemented in that case to understand what scope mappedTwice belongs to.

staltz commented 7 years ago

@hermanbanken I'm not so sure I understand, pardon me if I say something that doesn't make sense.

As far as I understand,

window.myObservable
  .switchMap(function(x) {
    return Rx.Observable.of(x); // <----- this line
  })
  .subscribe(function(x) {
    console.log(x);
  });

runs in the same window context as the switchMap:

window.myObservable
  .switchMap(function(x) { // <----- this operator
    return Rx.Observable.of(x);
  })
  .subscribe(function(x) {
    console.log(x);
  });

and the switchMap receives window.myObservable as the source Observable, which is the this here, expected to have the lift method.

hermanbanken commented 7 years ago

They run in the same window context, but originate from a different window I believe. Let's differentiate between window A and window B, with A.opener == B.

The window.myObservable is from context B (window.myObservable = window.opener.myObservable;) and the switchMap method is as a result from the prototype of Observable from context B.

// running in window A

window.myObservable  // <---- from context B, previously assigned
  .switchMap(function(x) {  // <---- from context B
    let X = Rx.Observable.of(x); // <----- run in window A, using Rx from context A
    return X;
  })
  .subscribe(function(x) {
    console.log(x);
  });

switchMap uses lift from context B because its prototype is from B. For the object X we need to check whether it (a) has a subscribe method or (b) is a Promise, or (c) all other things rxjs supports.

Or am I mistaken?

staltz commented 7 years ago

Understood, you're correct. And I just went through many operators and I don't think we will have a case where we would need to duck-type check for the existence of lift.

hermanbanken commented 7 years ago

Ah good to see that I was not just going crazy 😜 .

I think this is related to #2489 too, since things would get more complicated if we do this:

import switchMap from 'rxjs/somewhere/switchMap'

window.myObservable  // <---- from context B, previously assigned
  .pipe(switchMap(function(x) {  // <---- pipe from context B, switchMap from context A
    let X = Rx.Observable.of(x); // <----- run in window A, using Rx from context A
    return X;
  }))
  .subscribe(function(x) {
    console.log(x);
  });

at that point switchMap is from context B and for everything done during the setup phase of the Observable it can only assume valid what is from context B, while it is asked to work on something from context A, through pipe.

benlesh commented 7 years ago

A few things:

  1. instanceof checking is much faster than property look-ups.
  2. We'd need to check for more than just subscribe here. The instanceof check ensures that this is an Observable from the same library with all of the same properties... this includes optimizations like _isScalar, and the presence of important pieces of infrastructure like lift. And even if so, there's no guarantee it's the same version with the same semantics.

You can solve the problem outlined in your original post with a simple Observable.from:

if (window.opener) {
  window.myObservable = Rx.Observable.from(window.opener.myObservable);
} else {
  window.myObservable = Rx.Observable.of(1, 2, 3);
  window.open(location.href);
}

Covering this corner case that has a simple workaround is probably not worth the performance cost.

goldsam commented 7 years ago

@benlesh Observable.from has a problem similar to the original issue due to the observable symbol not evaluating equal across window contexts. The first conditional in the following excerpt from FromObservable.ts below always evaluates false:

...
if (typeof ish[observable_1.observable] === 'function') {
  if (ish instanceof Observable_1.Observable && !scheduler) {
    return ish;
  }
  return new FromObservable(ish, scheduler);
} ...

Do you see a way to easily work around this without library support?

roysudi commented 7 years ago

@benlesh Any help on this would be appreciated, trying to get the exact same piece work for my app - ng4.0, rxjs 5.0.2

marceloemanoel commented 6 years ago

Hi everyone any update on this?

marceloemanoel commented 6 years ago

If it can be of any help, I couldn't make it work with

Observable.from(originalObservable);

But I got it working with

Observable.create(observer => originalObservable.subscribe(observer))
benlesh commented 6 years ago

@marceloemanoel your workaround is the best I've seen for this so far. Unfortunately, we as a library can't just declare that everything with a subscribe method on it is an observable. There are plenty of APIs out there with subscribe that has a totally different signature (Redux or RxJS 4, for example)

I think your workaround will be as good as it can get for now.

lock[bot] commented 6 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.