mobx-utils icon indicating copy to clipboard operation
mobx-utils copied to clipboard

toStream is not rxjs compatible

Open meelkor opened this issue 4 years ago • 6 comments

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

meelkor avatar Jul 29 '21 17:07 meelkor

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

ObservedObserver avatar Aug 10 '21 09:08 ObservedObserver

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.

mweststrate avatar Aug 16 '21 20:08 mweststrate

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

quolpr avatar Aug 18 '21 06:08 quolpr

@benlesh does the fix look good?

benjamingr avatar Aug 30 '21 07:08 benjamingr

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?

benlesh avatar Aug 30 '21 12:08 benlesh

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.

TimonVS avatar Jan 25 '23 11:01 TimonVS