Skip to content

Observables cannot be passed between window contexts. #2628

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

Closed
goldsam opened this issue May 29, 2017 · 20 comments
Closed

Observables cannot be passed between window contexts. #2628

goldsam opened this issue May 29, 2017 · 20 comments

Comments

@goldsam
Copy link

goldsam commented May 29, 2017

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

goldsam pushed a commit to goldsam/rxjs that referenced this issue May 30, 2017
eliminate use of instanceof to support passing observables between javascript contexts

Closes ReactiveX#2628
@kwonoj
Copy link
Member

kwonoj commented May 30, 2017

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
Copy link
Author

goldsam commented May 30, 2017

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
Copy link
Member

staltz commented May 30, 2017

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
Copy link
Collaborator

@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
Copy link
Contributor

hermanbanken commented Jun 2, 2017

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
Copy link
Member

staltz commented Jun 3, 2017

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
Copy link
Contributor

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
Copy link
Member

staltz commented Jun 3, 2017

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
Copy link
Contributor

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
Copy link
Member

staltz commented Jun 14, 2017

@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
Copy link
Contributor

hermanbanken commented Jun 14, 2017

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
Copy link
Member

staltz commented Jun 14, 2017

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
Copy link
Contributor

hermanbanken commented Jun 14, 2017

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
Copy link
Member

benlesh commented Jun 14, 2017

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
Copy link
Author

goldsam commented Jun 23, 2017

@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
Copy link

roysudi commented Jun 23, 2017

@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

Georift added a commit to Georift/angular-cesium that referenced this issue Jan 18, 2018
Changes the instanceof check for rxjs to a duck-typing isObservable
method to fix mistakenly rejecting Observables from other contexts.

Window context issue: ReactiveX/rxjs#2628
@marceloemanoel
Copy link

Hi everyone any update on this?

@marceloemanoel
Copy link

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
Copy link
Member

benlesh commented Jan 30, 2018

@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.

@benlesh benlesh closed this as completed Jan 30, 2018
@lock
Copy link

lock bot commented Jun 6, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
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

8 participants