mobxjs / mobx-utils

Utility functions and common patterns for MobX
MIT License
1.18k stars 124 forks source link

toStream is not rxjs compatible #300

Open meelkor opened 3 years ago

meelkor commented 3 years ago

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 commented 3 years ago

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

mweststrate commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@benlesh does the fix look good?

benlesh commented 3 years ago

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 commented 1 year ago

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.