Test and refactor rendering

[pittst3r]

In order to test rendering (which is currently async) we needed to return a promise from scheduleRerender and by extension renderComponent. What do y'all think about this?

cc @chancancode @tomdale

[pittst3r]

This is what allows us to get rid of _rendered.

[chancancode]

Can you add tests for rendering multiple components, rendering into tags with existing content (with or without nextSibling), etc?

[pittst3r]

@chancancode Just added a bunch of tests. Let me know how this looks.

[tomdale]

scheduleRerender is a potentially very hot path, so we need to be careful not to do anything to deopt it.

I'm OK with this returning a promise, but:

1. Should we have an optimized path that doesn't allocate a Promise? The reality is that this is getting called potentially every rAF and the internals don't require the promise. We should avoid the allocation if we don't actually need it.
2. We should continue to maintain a Boolean flag, and make the check more explicit, c.f. https://twitter.com/michaelhaeu/status/845003383153025024 and More specific if conditions lead to ~10% faster render. preactjs/preact#610.

[chancancode]

@robbiepitts @tomdale

Re: (2) the micro-optimization from Benedikt's slide isn't actually referring to this case (and it likely won't matter here). If we care about that, the right thing to do here is to check for (this._scheduled !== null). Maintaining an parallel boolean field would likely turn out to be more costly (again, I doubt that matters at all – whatever makes the code easy to work with here wins IMO).

We can do (1) though. Something like this would probably work:

function NOOP() {}

class Application {
  private rerender: () => void = NOOP;
  private afterRender: () => void = NOOP;
  private renderPromise: Option<Promise<void>>;

  constructor() {
    // ...

    // This, or maintain the `rendered` flag, etc
    this.renderPromise = new Promise<void>(resolve =>{
      this.afterRender = resolve;
    });
  }

  render() {
    // ...
    this.env.commit();

    let renderResult = result.value;

    this.rerender = () => {
      this.env.begin();
      renderResult.rerender();
      this.env.commit();
      this.didRender();
    };

    this.didRender();
  }

  didRender(): void {
    let { afterRender } = this;

    this.afterRender = NOOP;
    this.renderPromise = null;

    afterRender();
  }

  scheduleRerender(): Promise<void> {
    let { renderPromise } = this;

    if (renderPromise === null) {
      renderPromise = this.renderPromise = new Promise<void>(resolve =>{
        this.afterRender = resolve;
      });

      this._scheduleRerender();
    }

    return renderPromise;
  }

  // internal version used by @tracked, etc
  _scheduleRerender() {
    if (this.renderPromise !== null) {
      requestAnimationFrame(this.rerender);
    }
  }
}

Does that make sense?

[pittst3r]

Just updated this per @chancancode's revision. I feel like I'm missing something though. I'm not seeing a path that avoids the Promise allocation. You can call _scheduleRerender once after the initial render, but then after that it's a noop.

[pittst3r]

My latest commit adds the promise-free path along with a couple boolean flags that seemed necessary to me. Essentially, _scheduleRerender has turned into the old scheduleRerender and the new scheduleRerender wraps it in a promise.

[chancancode]

Looks good to me! @krisselden might be able to comment on what patterns are good/bad performance-wise, but that could happen async and we don't need to block on that. @tomdale seems good to you?

[tomdale]

This looks good. My one request is that sooner rather than later we think generally about how we want to expose global rendering/lifecycle events. There are a number of things that are currently internal that people might want for testing, analytics, performance measuring, etc. I'd prefer to think about the problem holistically instead of accreting promise-based APIs for every use case that crops up.