bikeshaving / crank

The Just JavaScript Framework
https://crank.js.org
MIT License
2.7k stars 75 forks source link

async rendering of children: should be properly coordinated #68

Open ged-odoo opened 4 years ago

ged-odoo commented 4 years ago

Dear Author of Crank, as requested in a reddit thread (reddit ), here is a small scenario that I think is not handled properly by crank:

/** @jsx createElement */
import { createElement } from "@bikeshaving/crank";
import { renderer } from "@bikeshaving/crank/dom";

function wait(i) {
  return new Promise(resolve => {
    setTimeout(resolve, i);
  })
}
async function A({i}) {
  await wait(1000);
  return <div>A:{i}</div>;
}

async function B({i}) {
  await wait(200);
  return <div>B: {i}</div>;
}

function *Parent() {
  let i = 1;
  this.addEventListener('click', () => {
    i++;
    this.refresh();
  });
  while (true) {
    yield <div><A i={i}/><B i={i}/></div>;
  }
}
renderer.render(<Parent />, document.body);

We have here a parent with two sub components A and B. These sub components render themselves at a different speed. Whenever I click on the parent, some information is propagated to the full sub tree. What happens is that we see that B is updated first, and then A. I think that it is wrong. Both should be updated at the same moment. Otherwise, the UI is not in a consistent state.

brainkim commented 4 years ago

The name for this type of bug that I’ve heard is “tearing,” and I think you might find some helpful info if you search for that term in the other framework issue trackers.

Here’s a sandbox with your code. I guess, my response would be that this is the correct behavior for those two components. Typically, you should lift your async/stateful logic into as high an ancestor if you can, same as React, and if you need to synchronize two components you should do so in a shared parent with Promise.all:

async function* Parent() {
  let i = 1;
  this.addEventListener("click", () => {
    i++;
    this.refresh();
  });
  for await (const _ of this) {
    await Promise.all([wait(1000), wait(200)]);
    yield (
      <div>
        <A i={i} />
        <B i={i} />
      </div>
    );
  }
}

I guess if I knew more about the actual async calls you’re trying to synchronize I could offer additional suggestions, but that’s what I suggest for your current example.

ged-odoo commented 4 years ago

First, feel free to close this issue whenever you want. I did not come here with the goal to troll or to annoy you.

I understand your answer. But I think that it is a consequence of a "weak" asynchronous model: you push the coordination logic to the application layer. Your answer give the responsability to the end developer of making it work.

This means that:

  1. I may need to rewrite my code to push some logic out of a component (where it makes sense) and into another (where it may not make sense)
  2. it could also be impossible/very hard. For example, if one of these two components is a library.

Note that all standard frameworks currently have the same issue. They require the developer to write/organize the code around the framework's limitation.

This goes against the "component" model: my component leaks into the parent. And it is really hard when you have a modular system where the parent does not know its children component beforehand (plugin/extensible system).

If you are interested by the discussion, let me give you a more concrete example, which is the exact same scenario: we have 3 components, App, Record, Messages, where App is the main component, which display two sub components next to each other,:

The App component can switch record. Then, it gives a record id to each of the two subcomponents (Record and Messages). Now, if I write the code in the logical way, Crank will first update one of these two components, then the other one. This is really not what we want.

And moving the asynchronous logic out of these two sub components is not what we want either. The problem goes deep, and if we have a complex tree of components, then we need to do some complex plumbing at each component to coordinate it with its children... (been there, done that)

brainkim commented 4 years ago

@ged-odoo Don’t think you’re trolling, and you’re definitely not annoying me (yet 😈). I agree that it would be nice for parents to coordinate child components to appear together. The most advanced work on this stuff currently is the work by the React team with SuspenseLists. SuspenseList provides a revealOrder prop, which allows you determine the order in which components are revealed. I think there might be similar solutions in Crank, and my hope is that the async algorithms are simple enough that this kind of stuff can be implemented in user space. I’ll experiment and get back to you.

ged-odoo commented 4 years ago

Well, I would say that my own framework is more advanced than react then :)... For your information, we actually tackled this very problem last year. Our initial design was using internally promises, probably similar to what Crank does internally. But it could not work in many more advanced cases. Situations where:

That last scenario in particular was close to impossible, because we cannot really properly replace promises inside a Promise.all(...).

Our solution was to introduce a scheduler (about 100 loc), and an internal data structure (called Fiber) to track the state of all renderings. The scheduler simply checks for a completed task every animation frame, and if a fiber is completed, patch it to the DOM. These fibers simply track a number, which is incremented while it is waiting for a sub component to rerender, and decremented once it is ready.

nikordaris commented 4 years ago

Would something like this just work? Components are just functions after all?

async function* Parent() {
  let i = 1;
  this.addEventListener("click", () => {
    i++;
    this.refresh();
  });
  for await (const _ of this) {
    const A = await A({i});
    const B = await B({i});
    // Or Promise.all(...)
    yield (
      <div>
        <A />
        <B />
      </div>
    );
  }
}

If the components A and B were generators instead of just a async functions then you'd probably have to iterate over them. I can't remember if there is a way to combine two generators like you would with Promise.all for async functions.

If this isn't possible your library could just expose smart and dumb components and businesses logic state accessors not unlike the withFoo React Hooks. Probably would adopt a different naming convention than with to avoid confusion but you get the idea. Separating BL from view is a good idea in general regardless of using crank or react.

async function* Parent() {
  let i = 1;
  this.addEventListener("click", () => {
    i++;
    this.refresh();
  });
  for await (const _ of this) {
    const aState = await withA({i});
    const bState = await withB({i});
    // Or Promise.all(...)
    yield (
      <div>
        <A {...aState}/>
        <B {...bState}/>
      </div>
    );
  }
}
brainkim commented 4 years ago

@nikordaris I’m personally against calling functions meant to be Crank components directly. It’s just too complicated and error-prone for the caller because you have to handle the four component return types (T, Promise<T>, Generator<T>, AsyncGenerator<T>), and you also would not be passing in a context as the this keyword (there is no way currently to create a Crank Context object directly). So components are “just functions,” but Crank does a lot of work to call them in specific ways to make them components. Maybe the tagline “Just Functions” is misleading similar to the previous “Just JavaScript” tagline 😓.

nikordaris commented 4 years ago

That makes sense. I'm curious if there is a way to expose some utilities so we can manually render the components in the order we like and then stitch the results into the yielded jsx. Basically like the example I provided but having crank render it. Something like const A = await this.render(A, {i}) that way we can leverage this context and let the component control the children sequence?

brainkim commented 4 years ago

@nikordaris

That makes sense. I'm curious if there is a way to expose some utilities so we can manually render the components in the order we like and then stitch the results into the yielded jsx. Basically like the example I provided but having crank render it. Something like const A = await this.render(A, {i}) that way we can leverage this context and let the component control the children sequence?

It’s going to be cumbersome but you could probably already get this done with the API crank exposes.

async function *MyComponent({children}) {
  this.schedule(() => this.refresh());
  const div = yield <div>;
  for ({children} of this) {
    await renderer.render(children, div);
    yield <Copy />;
  }
}

You can call the renderer.render method directly in components, and it will return a promise of the component tree is async. If you want, you can also inject DOM nodes directly into the tree using a Raw element.

If you have more questions on this, it might be better to put it in a different issue just to keep this current issue clean.

Raiondesu commented 3 years ago

It should possible to create higher-order-components for this exact purpose:

// Async component factory that returns a stateless sync component
async function A({i}) {
  await wait(1000);
  return function (this: Context) {
    return <div>A:{i}</div>;
  }
}

async function B({i}) {
  await wait(200);
  return function (this: Context) {
    return <div>B: {i}</div>;
  }
}

async function *Parent() {
  let i = 1;
  this.addEventListener('click', () => {
    i++;
    this.refresh();
  });
  for await (const _ of this) {
    const A = await A({i});
    const B = await B({i});

    yield <div><A/><B/></div>;
  }
}

Didn't have a chance to check if this works, though.