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

toStream is not rxjs compatible #300

Open
meelkor opened this issue Jul 29, 2021 · 6 comments
Open

toStream is not rxjs compatible #300

meelkor opened this issue Jul 29, 2021 · 6 comments

Comments

@meelkor
Copy link

meelkor commented Jul 29, 2021

I noticed that the Symbol.observable, that should contain itself, is commented in the IObservableStream, which breaks compatibility with latest rxjs from function.
https://github.com/mobxjs/mobx-utils/blob/master/src/observable-stream.ts#L22
Resulting in Argument of type 'IObservableStream<...>' is not assignable to parameter of type 'ObservableInput<...>'

If I understand this correctly, the Symbol.observable in the observable streams is accepted by the community as a standard of a sort. Is there a reason the line is commented?

Versions:

mobx-utils: 6.0.4
rxjs: 7.3.0
@ObservedObserver
Copy link

I met the same problem when using rxjs7 with mobx-utils

@mweststrate
Copy link
Member

Feel free to uncomment, test and submitting a PR! I haven't used rxjs in a very long time, but sounds like a good change.

@quolpr
Copy link

quolpr commented Aug 18, 2021

I made custom to rxjs observer converter:

import { autorun } from 'mobx';
import { Observable } from 'rxjs';

export const toObserver = <T extends any>(obj: () => T) => {
  return new Observable<T>(function (observer) {
    const dispose = autorun(() => {
      observer.next(obj());
    });

    return () => dispose();
  });
};

Not sure about gotchas, but for me, it works well

@benjamingr
Copy link
Member

@benlesh does the fix look good?

@benlesh
Copy link

benlesh commented Aug 30, 2021

Maybe toObservable, but otherwise, yeah.

It's really early here so maybe I'm just tired, but looking at the code in question I'm not entirely sure what was wrong with it. It's only supposed to handle observer, but it handles observer and function. It doesn't seem to be broken?

@TimonVS
Copy link

TimonVS commented Jan 25, 2023

While toStream currently does seem to work when you ignore the incompatible type signature, there's another compatibility issue hiding in the implementation.

mobx-utils reevaluates Symbol.observable every time toStream is called, while RxJS evaluates it once and then caches the value (https://github.com/mobxjs/mobx-utils/blob/master/src/observable-stream.ts#L6 vs https://github.com/ReactiveX/rxjs/blob/366b0294744a3ceb270e180952dd5a155be3157b/src/internal/symbol/observable.ts#L2). This leads to the following error when there's some other library (or in our case, a live chat script) loads in a Symbol.observable polyfill:

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

I've created a Stackblitz to reproduce the issue: https://stackblitz.com/edit/typescript-5le5kn?file=index.ts (make sure to open the console).

Should I file a separate issue on this? I'm not sure if this should be fixed on the RxJS side or mobx-utils, my hunch is that the implementation on RxJS's side could cause compatibility issues with other observable libraries too. Update: it seems like mobx-utils is the odd one out here. Both zen-observable and XStream evaluate Symbol.observable only once.

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

7 participants