Onionify not working with RxJS

[ntilwalli]

Currently the StateSource assumes this.state$ has a compose method, but that is not the case when adapt gets called with a non-xstream library.

https://github.com/staltz/cycle-onionify/blob/master/src/index.ts#L111

this.state$ = adapt(stream.compose(dropRepeats()).remember());

The above line returns a stream in the app's chosen stream lib, so when select gets called on the StreamSource, this line fails since this.state$ is not an xstream stream.

https://github.com/staltz/cycle-onionify/blob/master/src/index.ts#L121

    return new StateSource<R>(
      this.state$.map(get).filter(s => typeof s !== 'undefined'),  
      null,
    );

I could create a PR with this change which I think will fix the problem...

export class StateSource<T> {
  public state$: MemoryStream<T>;
  private _name: string | null;

  constructor(stream: Stream<any>, name: string | null) {
    this._name = name;
    this._state$ = stream.compose(dropRepeats()).remember()
    this.state$ = adapt(this._state$);
    if (!name) {
      return;
    }
    (this._state$ as MemoryStream<T> & DevToolEnabledSource)._isCycleSource = name;
  }

  public select<R>(scope: Scope<T, R>): StateSource<R> {
    const get = makeGetter(scope);
    return new StateSource<R>(
      this._state$.map(get).filter(s => typeof s !== 'undefined'),
      null,
    );
  }

  public isolateSource = isolateSource;
  public isolateSink = isolateSink;
}

[goodmind]

@ntilwalli RxJS have let operator which same as compose
And all streams have map and filter which is why we don't need adapt here

UPDATE: sorry I got what you mean
There was xs.fromObservable initially #18 (comment)

[staltz]

Yes this makes sense @ntilwalli, and indeed the this.state$ should be of the type of the stream library used in your app. Using xs.fromObservable and adapt() are key here.

[ntilwalli]

I don't use the dev tool but I get the impression the dev tool assumes an xstream type stream for this.state$ so changes to this lib should coincide with changes there as well, yes?

[staltz]

The DevTool only supports xstream, this is because we rely on the fact that everything is hot in order to traverse the non-ambiguous chain of operators. Am I missing something? I don't see why we would need to change the DevTool.

[ntilwalli]

My only point was that this.state$ will no longer be guaranteed to be an xstream Stream, since it will be adapted, so tools which rely on that guarantee/assumption may need to be updated, but based on what you just stated, that's not necessary.

[staltz]

I could do this code change myself, but do you want to have the honor of the PR since you mentioned it first (in case you have the branch waiting to be sent or something)?

[ntilwalli]

@staltz thx. Please go ahead, I don't have a branch set up yet, I'm using a hack right now. I can def submit PR for review if that works better for you.

[staltz]

Fixed and published in v3.3.0