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 incorrectly defines observable symbol #4415

Closed
kiyui opened this issue Dec 10, 2018 · 9 comments · Fixed by #5874
Closed

RxJS incorrectly defines observable symbol #4415

kiyui opened this issue Dec 10, 2018 · 9 comments · Fixed by #5874
Labels
bug Confirmed bug

Comments

@kiyui
Copy link

kiyui commented Dec 10, 2018

Bug Report

It would appear that the current way RxJS detects other observables is not working as expected, while other libraries can adapt from each other.

Current Behavior
The from function in RxJS 6.3.3 does not seem able to adapt xstream and most while the older RxJS 5 Observable.from was able to. The following error is reproduced in both cases:

TypeError: You provided an invalid object where a stream was expected. You can provide an Observable, Promise, Array, or Iterable.

Both xstream and most provide Symbol polyfills:

Additional testing has confirmed:

  • This issue does not occur in webpack but it does in node, jest, and browserify.
  • The order of import seems to matter.

Reproduction

  • From xstream to rxjs, does not work:
const { from: observableFrom } = require('rxjs')
const { isInteropObservable } = require('rxjs/internal/util/isInteropObservable')
const { default: xs } = require('xstream')

const num$ = xs.from([ 1, 2, 3, 4 ])
console.log('Is num$ an observable?', isInteropObservable(num$)) // false

const adapt$ = observableFrom(num$) // error here
adapt$.subscribe(console.log)
  • From xstream to rxjs, working after importing xstream first:
const { default: xs } = require('xstream')
const { from: observableFrom } = require('rxjs')
const { isInteropObservable } = require('rxjs/internal/util/isInteropObservable')

const num$ = xs.from([ 1, 2, 3, 4 ])
console.log('Is num$ an observable?', isInteropObservable(num$)) // true

const adapt$ = observableFrom(num$)
adapt$.subscribe(console.log)
  • The same behaviour in most, not working:
const { from: observableFrom } = require('rxjs')
const { isInteropObservable } = require('rxjs/internal/util/isInteropObservable')
const most = require('most')

const num$ = most.from([ 1, 2, 3, 4 ])
console.log('Is num$ an observable?', isInteropObservable(num$)) // false

const adapt$ = observableFrom(num$) // error here
adapt$.subscribe(console.log)
  • We cannot convert rxjs to xstream either unless we import xstream first:
const { from: observableFrom } = require('rxjs')
const { default: xs } = require('xstream')

const num$ = observableFrom([ 1, 2, 3, 4 ])
const adapt$ = xs.from(num$)

adapt$.addListener({
  next: console.log
})

Expected behavior
The order of import should not matter, and the jest environment would work like webpack.

Environment

  • Runtime: Node v11.3.0
  • RxJS version: 6.3.3
  • xstream version: 11.7.0
  • most version: 1.7.3

Possible Solution

  • The file rxjs/src/internal/symbol/observable.ts could be defined the same way the other libraries do it:
Symbol('observable')
  • The isInteropObservable could be patched to check for the same symbol xstream and most use?
  • A similar polyfill to xstream or most could be included?

Additional context/Screenshots

  • This issue came about while working on a cyclejs project with rxjs.
  • Adding the following to jest.setup.js seems to have fixed the issue for now:
import xs from 'xstream' // eslint-disable-line

Thank you and sorry in advance if I've diagnosed the wrong source 🙇‍♂️

@kwonoj
Copy link
Member

kwonoj commented Dec 10, 2018

Need some digging, esp why our interop utility doesn't work based on import order.

couple of clarifications meanwhile

could be patched to check for the same symbol xstream and most use?

I think we already do, need to figure out why it doesn't work.

A similar polyfill to xstream or most could be included?

from v6, we intentionally removed polyfilling symbol for our own but only check if symbol exists.

@cartant
Copy link
Collaborator

cartant commented Dec 10, 2018

Need some digging, esp why our interop utility doesn't work based on import order.

If RxJS loads and there is no Symbol.observable, the string '@@observable' is used instead.

If xstream loads and there is no Symbol.observable, it assigns Symbol('observable') to Symbol.observable. xstream only uses '@@observable' if Symbol does not exist.

@kwonoj
Copy link
Member

kwonoj commented Dec 10, 2018

Don't we revalidate existense of symbol each time time when fn called? I thought we did that.

@cartant
Copy link
Collaborator

cartant commented Dec 10, 2018

@kwonoj
Copy link
Member

kwonoj commented Dec 10, 2018

Uh welp. 😅

@kwonoj kwonoj added the bug Confirmed bug label Dec 10, 2018
@pbadenski
Copy link

Any updates on this? I faced this issue with refract-rxjs which revalidates existence of symbol. It brought a hard to track bug that started to occur when I reordered my imports.

@shesek
Copy link

shesek commented Jul 1, 2019

I'm experiencing this too, unable to convert xstream streams to rxjs streams.

@benlesh
Copy link
Member

benlesh commented Jul 6, 2020

Digging into this:

It looks like the issue is we're not using unique symbol types with our typings. Furthermore, everyone else is using symbol-observable, which we could probably use as well, after we update it to also use unique symbol in it's types.

benlesh/symbol-observable#41

In the end, I'm not sure what we should do here to fix the typings though. It seems like once something is imported from another module or otherwise stuffed in a variable, TypeScript gets rather confused about what works and what doesn't.

@benlesh
Copy link
Member

benlesh commented Jul 8, 2020

There is another issue in the original post here, which is that, really, polyfills should always be applied before any libraries are loaded. So the runtime fix would be to just ensure the polyfilling happened first before the other imports.

The TypeScript issue I'm not sure we can fix at the moment.

I'm closing this for now, but I'm willing to reopen this later if this becomes a major issue.

This is related to issue #3890, and there's a solid workaround for the typings issue

The runtime issue can be resolved with a simple polyfill or just changing the order of imports.

// Running this code before any of your imports will resolve any runtime issues.
if (typeof Symbol === 'function') {
  if (!Symbol.observable) {
    Symbol.observable = Symbol('observable');
  }
}

@benlesh benlesh closed this as completed Jul 8, 2020
@benlesh benlesh reopened this Nov 2, 2020
benlesh added a commit to benlesh/rxjs that referenced this issue Nov 2, 2020
benlesh added a commit that referenced this issue Nov 2, 2020
Resolves #5861
Resolves #4415

BREAKING CHANGE: `rxjs@7` is only compatible with `@types/node@14.14.3` or higher and `symbol-observable@3.0.0` and heigher. Older versions of `@types/node` incorrectly defined `Symbol.observable` and will be in conflict with `rxjs` and `symbol-observable@3.0.0`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants