xstream: Design problems with async stop

Because of issues https://github.com/cyclejs/cyclejs/issues/365 and https://github.com/staltz/xstream/issues/90, I made a fix to xstream which gives special treatment to a corner case in flatten, which was released in v5.3.2. https://github.com/staltz/xstream/commit/819bc94883eb32f0debac5f29468e609a62416fa

However, (1) it doesn’t actually fix the problem, (2) it conflicts with previously desired features. It also makes new bugs appear such as #103.

(1)

Note how we added this test, which passed in v5.3.2:

    it('should restart inner stream if switching to the same inner stream', (done) => {
      const outer = fromDiagram('-A---------B----------C--------|');
      const nums = fromDiagram(  '-a-b-c-----------------------|', {
        values: {a: 1, b: 2, c: 3}
      });
      const inner = nums.fold((acc, x) => acc + x, 0);

      const stream = outer.map(() => inner).flatten();

      const expected = [0, 1, 3, 6, 0, 1, 3, 6, 0, 1, 3, 6];

      stream.addListener({
        next: (x: number) => {
          assert.equal(x, expected.shift());
        },
        error: (err: any) => done(err),
        complete: () => {
          assert.equal(expected.length, 0);
          done();
        }
      });
    });

However, this does not pass in v5.3.2:

    it('should restart inner stream if switching to the same inner stream', (done) => {
      const outer = fromDiagram('-A---------B----------C--------|');
      const nums = fromDiagram(  '-a-b-c-----------------------|', {
        values: {a: 1, b: 2, c: 3}
      });
      const inner = nums.fold((acc, x) => acc + x, 0);

-     const stream = outer.map(() => inner).flatten();
+     const stream = outer.map(() => inner.map(x => x)).flatten();

      const expected = [0, 1, 3, 6, 0, 1, 3, 6, 0, 1, 3, 6];

      stream.addListener({
        next: (x: number) => {
          assert.equal(x, expected.shift());
        },
        error: (err: any) => done(err),
        complete: () => {
          assert.equal(expected.length, 0);
          done();
        }
      });
    });

Because every time the function () => inner.map(x => x) is called, inner.map(x => x) will yield a different stream, where as () => inner would always yield inner as the same stream.

(2)

Sync start and async stop was designed to allow the inner stream to not restart if it was the same during the switch in a flatten. This was really by design, to avoid some confusion with a common pattern we had in Cycle.js, the use of RxJS connect() here: https://github.com/cyclejs/cyclejs/commit/67d176e3e8e6f96b776428fc55f5c29c81f54607#diff-32a0a3abed94d032137ef603ee4dd261L30

So “don’t restart the inner stream if it remains the same during flatten” is a feature by design.

However, to have referential transparency we want these two cases to give the same behavior:

    const inc$ = sources.DOM.select('.inc').events('click').mapTo(+1);
    const refresh$ = sources.DOM.select('.ref').events('click').startWith(0);
+   const sum$ = inc$.fold((x, y) => x + y, 0);
+   const lastSum$ = refresh$.map(_ => sum$).flatten();
-   const lastSum$ = refresh$.map(_ => inc$.fold((x, y) => x + y, 0)).flatten();
    const vdom$ = lastSum$.map(count =>
      div([

Which means we want the property “restart the inner stream if it remains the same during flatten” by design.

Which means we have a conflict, and we need to choose which of these two to do. It may mean a breaking change. I had hopes sync start and async stop would make things more intuitive but there is an obvious drawback that makes xstream less intuitive.

If you’re reading this thread, please give your friendly and thoughtful opinion on this topic. This appears to be my design mistake, but I’m just a human. What’s important is that I’m willing to look for a better way forward. My intent with sync start and async stop was to provide an “just works” experience for most cases, and together with Tylor we did a lot of predictions and bike-shedding, but a corner case slipped out of our sight.

For now, I’ll revert the bugfix that happened in v5.3.2, so that other issues don’t surface, and to keep a consistent behavior.