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

Broken interoperability between Rxjs and symbol-observable #38

Closed
ivstas opened this issue May 1, 2018 · 13 comments
Closed

Broken interoperability between Rxjs and symbol-observable #38

ivstas opened this issue May 1, 2018 · 13 comments

Comments

@ivstas
Copy link

ivstas commented May 1, 2018

Hi.
Due to latest decision to avoid polyfilling Symbol.observable and 6.0 release going public, there is a difference between how symbol observable is being defined in Rxjs and this repo.

Basically, there are 3 cases:

  1. old browsers (no Symbol at all)
  2. modern browsers(Symbol is presented, but Symbol.observable is not)
  3. bright future (when we have Symbol.observable in global scope).

The problem is that for case #2 implementation differs:

Why is it an issue:

3rd party libraries relying on symbol-observable package to retrieve Observable will fail to get one from RxJs.

Possible solution

One solution I might think of is to avoid altering the global scope and simply fallback to @@observable (like RxJs does). However, it's a big breaking change for this package.

@benlesh
Copy link
Owner

benlesh commented May 24, 2018

RxJS is now designed to have a polyfill loaded first, as polyfills should be.

@ivstas
Copy link
Author

ivstas commented May 25, 2018

@benlesh ok, and in this scenario (when libraries don't polyfill) this ponyfill should also avoid to alter the global scope.

@zerobias
Copy link

zerobias commented Jul 3, 2018

Users of symbol-observable always get the same symbol, even if there will be multiple instances of the library, thus they depend on symbol-observable only.

Your implementation doesn't normalize Symbol.observable, thus you depend on globals, based on the hypothetical future standard and order of importing polyfills.
And of course, that implies importing rx to get your symbol

@typeofweb
Copy link

You should include all polyfills first, before any libraries. If you do that, RxJS will pick up the just-polyfilled Symbol.observable and use it.

And this package should be renamed to polyfill as opposed to ponyfill to reduce confusion :)

@WearyMonkey
Copy link

Just got hit by this. We're using redux (via read-dnd), Rxjs and mobx-utils.

Rxjs and mobx-utils use a ponyfill. Redux uses this polyfill.

The behaviour in our build was:

  1. Rxjs ponyfill imported, Symbol.observable not available so '@@observable' used.
  2. symbol-observable imported via redux, Symbol.observable set.
  3. mobx-utils imported, Symbol.observable is now available so used.

Rxjs and mobx-util observables are no longer compatible with each other.

This behaviour is expected for a polyfill, as other comments suggest polyfills must be imported before all other modules.

However this module very clearly advertises itself as a ponyfill. But this module is not pure like a pony.

So I suggest either:

  1. Be a good pony and don't have any side effects.
  2. Don't advertise as a ponyfill.

Rxjs ponyfill: https://github.com/ReactiveX/rxjs/blob/master/src/internal/symbol/observable.ts#L13
mobx utils ponyfill:https://github.com/mobxjs/mobx-utils/blob/master/src/observable-stream.ts#L6

@tukwan
Copy link

tukwan commented Sep 17, 2018

How about ?

import { computed } from 'mobx'
import { Observable } from 'rxjs/Observable'
import { Observer } from 'rxjs/Observer'

export const toStream = <T>(expression: () => T): Observable<T> =>
  Observable.create((observer: Observer<T>) => {
    const computedValue = computed(expression)
    return computedValue.observe(change => observer.next(change.newValue))
  })

@juanger
Copy link

juanger commented Jan 18, 2019

I got bitten by this issue too in a setup similar to @WearyMonkey's (redux + rxjs).

In my case I got the error only in tests using the TestScheduler (from 'rxjs/testing') and only when takeUntil was used.

Adding import 'symbol-observable'; to the top of my webpack entrypoint solved the issue.

@bradfordlemley
Copy link

bradfordlemley commented Apr 17, 2019

You're advising random modules to use this module, meaning that random modules will polyfill the environment for me. No thanks. That's kinda mean.

import Symbol_observable from 'symbol-observable';
const Symbol_observable = Symbol && Symbol.observable || '@@observable';

@zerobias
Copy link

zerobias commented Apr 21, 2019

No thanks

Sure, interop and agreements are developed only to let you break them. Obviously, there isn't enough inconsistency in the web, let's create brand new polyfill per project basis

@bradfordlemley
Copy link

let's create brand new polyfill per project basis

const Symbol_observable = Symbol && Symbol.observable || '@@observable'; is not a polyfill.

If that's all this module did, it would be great, and I would use it.

But, this module is a polyfill, it sets window.Symbol.observable. And it's not just the principle ... it actually breaks things because the polyfill occurs at whatever random point some random module that uses this module gets included.

@zerobias
Copy link

In this issue, only rxjs breaks things, because bad interop is still better than a perfect disorder

@benlesh
Copy link
Owner

benlesh commented Nov 2, 2020

This has been resolved for some time. I tried again to see if I could replicate it by importing from rxjs or symbol-observable in various orders. All is well:

https://codesandbox.io/s/vigilant-shirley-ve266?file=/src/index.ts

@benlesh benlesh closed this as completed Nov 2, 2020
@jameslaneconkling
Copy link

jameslaneconkling commented May 18, 2021

I believe this is still an issue:

Import rxjs into an environment where Symbol.observable isn't defined, then later import symbol-observable. rxjs will use the string "@@observable" while symbol-observable will use the symbol symbol(observable).

E.g. https://codesandbox.io/s/lucid-wind-csjbo?file=/src/index.ts . Passing an observable built with symbol-observable to rxjs causes rxjs to throw b/c it doesn't recognize the passed observable as an observable.

the issue only shows up when:

  • running in an environment where Symbol.observable isn't defined
  • rxjs is imported before symbol-observable (or any library depending on symbol-observable) is imported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants