thi-ng / umbrella

⛱ Broadly scoped ecosystem & mono-repository of 199 TypeScript projects (and ~180 examples) for general purpose, functional, data driven development
https://thi.ng
Apache License 2.0
3.39k stars 150 forks source link

[hiccup-svg/rstream/rdom] Issues during update cycle using $klist #491

Closed arnaudchenyensu closed 2 months ago

arnaudchenyensu commented 2 months ago

Hi @postspectacular ,

I have a list of steps with each step having a list of IHiccupShape2 that can be selected and then drawn into a svg canvas. A shape can be selected and should be filled in blue if this is the case.

However, when switching step I get this error:

Uncaught (in promise) Error: illegal state: operation not allowed in state 3

Would it be possible to help me find my mistake? Here's my code:

import { convertTree, svg } from "@thi.ng/hiccup-svg";
import { $compile, $klist } from "@thi.ng/rdom";
import { reactive, sync, syncRAF } from "@thi.ng/rstream";
import { indexed } from "@thi.ng/transducers";
import { circle, type IHiccupShape2, asSvg } from "@thi.ng/geom";

const WIDTH = 400;
const R = 20;

const onclick = (e: MouseEvent) => {
  const shapeId = (e.target as any).id
  $selectedShapeId.next(parseInt(shapeId, 10))
}

type Step = IHiccupShape2[]
const $selectedStepIndex = reactive(0)
const $selectedShapeId = reactive(-1)
const $steps = reactive<Step[]>([
  [
    circle(
      [100, 100],
      R,
      {
        id: 0,
        style: {
          fill: $selectedShapeId.map(id => id === 0 ? 'blue' : '')
        },
        onclick
      }
    )
  ],
  [
    circle(
      [100, 100],
      R,
      {
        id: 0,
        style: {
          fill: $selectedShapeId.map(id => id === 0 ? 'blue' : '')
        },
        onclick
      }
    ),
    circle(
      [200, 200],
      R,
      {
        id: 1,
        style: {
          fill: $selectedShapeId.map(id => id === 1 ? 'blue' : '')
        },
        onclick
      }
    )
  ],
])

const $shapes = sync({ src: { $selectedStepIndex, $steps }}).map((src) => {
  const step = src.$steps[src.$selectedStepIndex]
  return step ? step : []
})

$compile([
  'div.flex',
  {},
  $klist(
    syncRAF($steps).map((steps) => [...indexed(0, steps)]),
    'div.pointer',
    { style: { margin: '10px' } },
    ([i, _shapes]) => [
      'div',
      {
        style: {
          backgroundColor: $selectedStepIndex.map((idx) => idx === i ? 'gray' : '')
        },
        onclick: () => $selectedStepIndex.next(i)
      },
      `Step ${i}`
    ],
  ),
  svg(
    {
      id: 'svg-canvas',
      style: {
        borderWidth: '1px',
        borderStyle: 'solid',
        borderColor: 'black',
        margin: '10px',
      },
      width: WIDTH,
      height: WIDTH,
      viewBox: `0 0 ${WIDTH} ${WIDTH}`,
    },
    $klist(
      $shapes,
      "g",
      {
        fill: 'white',
        stroke: 'black',
      },
      (shape) => convertTree(shape.toHiccup()),
      (shape) => asSvg(shape)
    ),
  ),
]).mount(document.getElementById("app")!)

Thank you in advance for your help, Arnaud.

postspectacular commented 2 months ago

Hey @arnaudchenyensu 👋 — okay, this took me a little to decypher & figure out myself, but the answer is completely obvious once I got a mental picture of your setup...

Firstly, the error message you're receiving relates to one of the rstream constructs having gone into the "unsubscribed" state, but since the error itself occurs inside an async function and you've got quite a few subscriptions, it's not trivial to figure out which one...

So I started commenting out all the subs for the fill attribs of your shapes. With these removed, the error disappears and you can freely switch between the steps ad infinitum. Then it quickly dawned on me what the problem is here:

  1. The app launches showing only the circle for step 0
  2. Switching to step #1 and because you're using $klist, it is figuring out it can keep keep the exisiting circle from step #0 (resolves to the same key), but then adds the other circle defined in step #1. So far so good...
  3. Switching back to step #0, again $klist figures it can keep the first circle, but now has to remove the second one. This also means, the subscription for that second circle's fill attrib has to be removed and that's exactly what it does. So far so good...
  4. Switching back to step #1 and trying to re-create that second circle now means in order for the fill attrib of that new circle to become reactive, rdom has to attach a new sub to apply any changes, but now things go very wrong (actually NOT wrong at all!)...

The default behavior of all rstream subscriptions is to recursively unwind & clean up a topology of connected subscriptions as far upstream as possible (to avoid any uneccessary work being done by unused subs and also to help the GC), for example:

const a = reactive(42);
const b = a.subscribe();
const c = b.subscribe();

// trigger recursive clean up & termination
c.unsubscribe();

c.getState() // 3 (aka State.UNSUBSCRIBED)
b.getState() // 3
a.getState() // 3

As you can see, because c completed, and because a and b only had a single subscriber each, c's unsubscription and cleanup propagated upstream and put the entire topology into a state in which none of the nodes allow any new future subscriptions to be attached...

c.subscribe()
// Uncaught Error: illegal state: operation not allowed in state 3

This knowledge is the key to your error which we can solve in multiple ways now (I will mention two here...). Just to clarify for your example: When rdom removed the 2nd circle, it also removed its own sub for that circle's fill attrib, which then triggered the sub you've defined for the fill attrib to unsubscribe itself too (because of default behavior)... The reason that $selectedStepIndex itself is not impacted is because of the remaining presence of the other circle. If that one wouldn't exist then $selectedStepIndex too would be terminated...

Regardless of what you're choosing, to be on the safe side, I'd recommend adding the following options to your main state var definitions to protect them from the above unwinding behavior (btw. I will soon replace the CloseMode enum with just string types for more brevity already done), also see CommonOpts docs:

// updated this example for latest rstream v9.0.0

const $selectedStepIndex = reactive(0, { closeOut: "never" });
const $selectedShapeId = reactive(-1, { closeOut: "never" });

One of the actual solutions to fix your error is to add the same option to all your fill subs, e.g. like so:

fill: $selectedShapeId.map((id) => (id === 0 ? "blue" : ""), { closeOut: "never" }),

An alternative solution would be to wrap the shape arrays in $step inside a no-arg function which you then call to obtain a new shape array from your $shapes sub each time the user switches between steps. That way the fill subscriptions are always new/fresh ones, rather than attempting to re-use them as you've been trying to do so far... either way works!

import type { Fn0 } from "@thi.ng/api";

// wrap the shapes for each step in a function
const $steps = reactive<Fn0<Step>[]>([
    () => [
        circle([100, 100], R, {
            id: 0,
            style: { fill: $selectedShapeId.map((id) => (id === 0 ? "blue" : "")) },
            onclick,
        }),
    ],
    () => [
        circle([100, 100], R, {
            id: 0,
            style: { fill: $selectedShapeId.map((id) => (id === 0 ? "blue" : "")) },
            onclick,
        }),
        circle([200, 200], R, {
            id: 1,
            style: { fill: $selectedShapeId.map((id) => (id === 1 ? "blue" : "")) },
            onclick,
        }),
    ],
]);

const $shapes = sync({ src: { $selectedStepIndex, $steps } }).map((src) => {
    const step = src.$steps[src.$selectedStepIndex];
    // call the function to produce new shape array
    return step ? step() : [];
});

Sorry for a rather longwided explanation, but I hope that way you better understand WHY this is going wrong, even if at first it might seem intractable...

arnaudchenyensu commented 2 months ago

Hi @postspectacular , as always thank you for your quick reply!

It makes more sense now, it would have been hard to find the solution by myself 😅

postspectacular commented 2 months ago

@arnaudchenyensu Yeah, I think so too — Absolutely nothing personal with you, instead I really see this is as a fundamental issue, yet also don't know how to really improve this. The default teardown behavior is of utmost importance and can't/shouldn't be changed, and the options mentioned above are also described both in the rstream readme & API docs, but I know that most people are not reading these (this same issue as you've been having has come up in different guises several times now). So if you have any ideas how this could be improved, I'm always ear! :)

Btw. As you might have read elsewhere I'm also still planning on removing rstream as dependency for rdom and switch the latter to just using native async iterables, which mostly offer similar functionality (but a little less powerful) than rstream. I'm still not sure, but some of these teardown aspects/issues might be overcome completely once that switch happens (and don't worry, you will still be able to use rstream constructs via the super lightweight asAsync() adapter, which might also be handled for you, will see...)

arnaudchenyensu commented 2 months ago

Hi @postspectacular,

I know that you said it multiple times that thi.ng is not a framework but I think that having at least one package that is a framework might help newcomers.

The main issue that I have right now with thi.ng is that the packages almost fit together but not quite. We already discussed a bit on one of my previous issue where I was trying to use SVG elements with rdom but the edge cases were a bit hard to understand to me. I had another similar issue where I was trying to display a shape using IHiccupShape2 and rdom using the toHiccup method but it turns out that I needed to use convertTree to actually make it work – I was just assuming that rdom could handle any kind of hiccup format.

And often when I'm trying to combine the packages together (at least rdom, rstream and geom) I'm facing issues that are not easy to understand in my opinion (I'm still having issues understanding $klist).

That's why I'm thinking that maybe having a framework that combines some packages to have a coherent workflow/user experience might be helpful. I even think that if the framework ends up being a small DSL it would be great (as an example, I love how simple and coherent/unified is Elm).

Obviously this is just my opinion but this is the issues that I'm facing right now when I'm trying to use thi.ng. If it wasn't clear or you want to discuss it further, I would be happy to have a quick chat or even a live programming session to show you how I navigate the documentation, my thought process, etc. when I'm using your project.

And related to the native async for rdom, do you have some examples? Which package is it? I'm quite curious. If I remember correctly, I think that you once said that you found fibers easier to use for UI? (I think it was in one of your mastodon's post? But I can't find it anymore).

As always, thank you for your great work! ❤️