bikeshaving / crank

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

Async generator component stops updating DOM if a sync component is yielded #185

Closed spiffytech closed 3 years ago

spiffytech commented 3 years ago

If an async generator component yields a sync component, rendering stops or breaks.

If the sync component is the first thing rendered, rendering displays the sync component and nothing afterwards. Further code/yields continue to run (console.log fires), but the DOM doesn't change.

(CodePen)

function MyComponent() {
  return Crank.createElement('p', {}, 'Yielded component')
}

async function* MyApp() {
  yield Crank.createElement(MyComponent);
  console.log("Performed component yield");

  yield Crank.createElement('p', {}, 'Third yield');
  console.log("Performed final yield");
}

Crank.dom.renderer.render(Crank.createElement(MyApp), document.getElementById('app'))

If I yield a built-in element (<p>) before yielding MyComponent, I get unpredictable broken behaviors in CodePen or in my project.

brainkim commented 3 years ago

There are three qualities of async generator components which jointly produce the surprising behavior in your experiments.

  1. Async generator components are continuously resumed. Think of it like this. Sync generator components work in lockstep with refresh() calls and parent updates, i.e. their execution is locked to the execution of callbacks/ancestors. On the other hand, async generator components have some degree of independence, and can yield new values regardless of what their ancestors are doing. This allows us to write components like the following.
async function *Timer() {
  let i = 0;
  while (true) {
    yield <div>Seconds: {i}</div>;
    await new Promise((resolve) => setTimeout(resolve, 1000));
    i++;
  }
}

This component updates independently of parents while it is mounted into the page. However, we typically want async generator components to have some relationship to parents/refresh() calls, so we put yields inside a for await…of this loop instead. This allows the async generator component to await new updates as async events themselves.

  1. Generator components render their return values and stop updating. Because there is no loop in your async generator component, the function exits. When that happens, Crank renders the returned value and never updates the component ever again. Because all functions in JavaScript implicitly return undefined for implicit returns, the result is that Crank renders nothing for the component. If you add a return statement to the bottom of the function which yields a div or something, you’d see that the component sticks to that final value.

  2. Async generator components resume immediately. The speed at which async generator components resume is based on the microtask queue. This means these components re-execute/re-render immediately and the duration is not perceptible to humans. Your async generator component yields <MyComponent />, <p/> and returns undefined all within the same microtask loop. If you want to see the individual steps of the async generator component, you can await a one second delay after each yield, and you’ll see that each step is properly rendered for that second.


You’re not the first to say this is surprising behavior (see https://github.com/bikeshaving/crank/issues/28). However, I stand by the implementation, insofar as these behaviors were implemented so that async generator components could be used as concurrency primitives which allow you to race multiple async trees for fallbacks and such.

As far as I can tell, I see no bugs in your code, but async generator components are kinda complicated and I’ve definitely introduced regressions in the past. Let me know if this response doesn’t explain away the surprising behavior, and I’ll take a look.

PS. If you’re look for a transpilation-less alternative to JSX, you may want to consider https://github.com/developit/htm, which works with Crank mostly. I have plans to bring it into Crank so it can be extended at some point but I’ve had a lot of fun using it with bundle-less projects.

spiffytech commented 3 years ago

Thanks for the detailed reply. Those behaviors make sense and I think they're good to have in Crank, though they can be tricky to use correctly.

I think in my attempt to boil down my real project's behavior to a minimal reproduction* maybe I accidentally tripped some other Crank.js behaviors that cause similar results? Here's the snippet from my actual project:

export default function Spinner(this: Context) {
  return (
    <div class="w-full flex justify-center">
      <img src="/img/spinner.gif" class="w-24 h-24" />
    </div>
  );
}

async function* App(this: Context) {
  const debug = Debug("laglime:app");
  // Not sure where I have to put the refresh to make the `for await` run
  // immediately one time
  this.schedule(() => {
    // This gets printed
    debug("Refreshing top...");
    this.refresh();
  });

  yield <Spinner />;

  debug("Creating session...");
  let { user } = await userbase.init({
    appId: "",
    sessionLength: 24 * 14, // hours
    updateUserHandler: ({ user: updatedUser }) => {
      debug("Received new user object");
      user = updatedUser;
      this.refresh();
    },
  });
  debug("Session created!");
  debug("Retrieved user: %O", user);

  //await new Promise((resolve) => setTimeout(resolve, 1000));

  this.addEventListener("logged-in", () => {
    this.refresh();
    page("/");
  });

  // This never displays
  yield <p>Past the spinner</p>;

  // Printing gets down here
  debug("Looping over props generator");
  this.schedule(() => {
    // This never prints
    debug("Refreshing bottom...");
    this.refresh();
  });
  for await (let props of this) {
    // This never prints
    debug("Looping");
    yield (
      <Fragment>
        <Nav />
        <main class="px-4">
          <Router state={{ username: user?.username }} />
        </main>
      </Fragment>
    );
  }
}

This snippet sounds like it addresses your three points:

  1. It uses for await... to gate refreshes on props changes after its initial yield
  2. It's in an infinite loop, so no implicit return from App
  3. It forces a trip out to the event loop with the await. It's a really fast trip (4ms), but it's there.

If I run this code in my project, I see my spinner stay there indefinitely. If I insert a Promise-based sleep statement for 1000ms after the userbase.init() block, I see the spinner for a second, then my actual layout renders as it should. But without the sleep, the spinner never goes away, even though I can see debug statements fire all the way down to just before the for await....

Am I still doing something wrong here?


* I hadn't seen HTM before, it looks really cool. My original snippet used createElement because it's what I could make work in a CodePen, but I'll definitely look into HTM.

brainkim commented 3 years ago

Hmmmmm. First thing that jumps out at me is that you have to make sure you refresh before yield operators, not after. If you schedule a refresh after, you’re adding the schedule() callback too late for the previous render, so you’re inadvertently causing a deadlock.

Second thing is that you only need to schedule a single refresh before the final yield operation before you enter the main for await loop. The for await loop will be suspended if you yield before it is entered. If you’re yielding multiple values before the loop is entered, there is no need to schedule a refresh until right before the final yield. So the idea is that refresh() doesn’t really control execution of the async generator component, just when the context async iterator which you for await over resumes.

See if the following reordering gets you closer to the behavior you want.

export default function Spinner(this: Context) {
  return (
    <div class="w-full flex justify-center">
      <img src="/img/spinner.gif" class="w-24 h-24" />
    </div>
  );
}

async function* App(this: Context) {
  const debug = Debug("laglime:app");
  // first schedule is redundant
  yield <Spinner />;

  debug("Creating session...");
  let { user } = await userbase.init({
    appId: "",
    sessionLength: 24 * 14, // hours
    updateUserHandler: ({ user: updatedUser }) => {
      debug("Received new user object");
      user = updatedUser;
      this.refresh();
    },
  });
  debug("Session created!");
  debug("Retrieved user: %O", user);
  this.addEventListener("logged-in", () => {
    this.refresh();
    page("/");
  });
  // we move the first schedule refresh before the last yield before the loop.
  this.schedule(() => this.refresh());
  yield <p>Past the spinner</p>;
  // just proving that past the spinner is rendered before we enter the loop.
  await new Promise((resolve) => setTimeout(resolve, 1000);
  // Printing gets down here
  debug("Looping over props generator");
  for await (let props of this) {
    // This should print
    debug("Looping");
    yield (
      <Fragment>
        <Nav />
        <main class="px-4">
          <Router state={{ username: user?.username }} />
        </main>
      </Fragment>
    );
  }
}

The behavior was originally implemented because I kept writing async generator functions which had setup code like you would have in sync generator components, but because async generator components immediately resume and the for await iterators iterated immediately, the setup would never show (see https://github.com/bikeshaving/crank/issues/137). The fact that schedule() callbacks need to be put precisely before the yield is an unfortunate pitfall which I hadn’t anticipated.

However, looking at your code, it seems like maybe what you want is to not have any scheduled refresh calls, and that the default behavior is what you want? In other words, you might only want to enter the main loop when the updateUserHandler or logged-in callbacks fire and causes a refresh, in which case it would be best to not schedule any refreshes at all.

spiffytech commented 3 years ago

Your reordered code fixes everything! 🎉 Thanks!

As I'm coming up to speed on Crank, snippets like this come from me basically wanting loading indicators and understanding Crank poorly enough that the docs' pattern seemed like a bad fit for my needs.

But reading that page with fresh eyes, and in the context of your above comments, the pattern makes a lot more sense to me, and seems to do what I'm after, after all:

async function* LoadingIndicator(
  this: Context,
  { children }: { children: Children }
) {
  // This will display the spinner + children combo once, but the children can
  // redraw themselves without kicking the spinner again
  //
  // 🌠 The more you know! 🌠
  for await (let props of this) {
    yield <Spinner />;
    yield children;
  }
}

async function* Layout(this: Context) {
  const debug = Debug("laglime:app");

  debug("Creating session...");
  let { user } = await userbase.init({
    appId: "",
    sessionLength: 24 * 14, // hours
    updateUserHandler: ({ user: updatedUser }) => {
      debug("Received new user object");
      user = updatedUser;
      this.refresh();
    },
  });
  debug("Session created!");
  debug("Retrieved user: %O", user);

  this.addEventListener("logged-in", () => {
    this.refresh();
    page("/");
  });

  for await (let props of this) {
    yield (
      <Fragment>
        <Nav />
        <main class="px-4">
          <Router state={{ username: user?.username }} />
        </main>
      </Fragment>
    );
  }
}

function App(this: Context) {
  return (
    <LoadingIndicator>
      <Layout />
    </LoadingIndicator>
  );
}

That feels much cleaner and more clearly articulates my intentions.

Thanks for your help! Crank feels a little finicky due to pitfalls like we discussed, but on balance I'm very happy with it so far, and excited to have a frontend framework much closer to vanilla JS than the household names. It's everything I liked about Mithril brought forward to modern practices.

brainkim commented 3 years ago

It's everything I liked about Mithril brought forward to modern practices.

@spiffytech I love mithril and respect any framework which bills itself as feature complete, so that’s a huge compliment!

understanding Crank poorly enough that the docs' pattern seemed like a bad fit for my needs.

I definitely need to keep iterating on the docs on async components, because some parts seem to be lost in translation. Documenting the nuanced behavior of async generator components has been very difficult, and there’s a lot about the interactions between the four component types which I haven’t yet been able to document. More diagrams are probably necessary. Suggestions are appreciated!

spiffytech commented 3 years ago

I think having Crank emit errors/warnings in more situations would go a long ways towards compensating for the docs' lack of nuance.

E.g., an async generator returning undefined doesn't strike me as a common thing to want to do, and if someone wants it maybe they should return null so it's explicit. Then Crank could throw an error if an async generator returns undefined. That should points the user in the right direction to debug things.

Or maybe if the user calls this.schedule() and the component doesn't yield for X seconds, emit a warning. That'd catch my mistake where I thought the refresh should get scheduled because the code executed the this.schedule line.

I'll bet there's some low-hanging fruit to make some assumptions about user intent a bit more explicit, and sprinkle throw / console.warn() around inside Crank when it has a hunch the user's not doing what they think they're asking for.