Add SuspenseList component

[prateekbh]

• - enable way for Suspense boundaries to be controlled externally(new hook).
• - create SuspenseList using above hook to control its children's rendering order.
• - create forwards mode for SuspenseList.
• - create backwards mode for SuspenseList.
• - create together mode for SuspenseList.
• - create default mode for SuspenseList.
• - create TS definitions SuspenseList.
• - enable nested SuspenseList.
• - create tests for SuspenseList.

[coveralls]

Coverage increased (+0.1%) to 100.0% when pulling 05fedea on suspense-list into 9764924 on master.

[andrewiggins]

Some initial comments. Excited to see this!!

[andrewiggins]

Hmm do we need this to be an option? Could we simplify and save some bytes and move the _parent._component._modifySuspense into Suspense directly?

[prateekbh]

Sadly no.

A. That makes it really coupled, but I get it can be worthy of the savings.
B. Suspense isn't the only one who needs this logic. A SuspenseList can itself be a inside other other SuspenseList, thus we need to share the logic at some external point therefore an "option".

e.g.

<SuspenseList>
  <SuspenseList>...<SuspenseList>
  <Suspense>...<Suspense>
</SuspenseList>

[andrewiggins]

Whoa! Nested SuspenseList 🤯 Gotta think more about that

[andrewiggins]

Unfortunately, find isn't in our browser matrix 😢 since we don't compile to ES6. However, you should be able to emulate this find call with .some and use the return value to break out of the loop.

For the others you can use .some and modify the first matching object in your callback when you find the match and then return true to break.

[jviide]

I chatted with @prateekbh about a parallel approach. If the _thrillers array's size is n then the worst-case amount of work is something like O(n^2), and some work could be avoided by using a singly linked list and a Map. Here's an incomplete sketch, sorry about that 🙂

export function SuspenseList(props) {
  this._head = null;
  this._vnodes = null;
}

SuspenseList.prototype.render = function(props, state) {
  // Points to the first node of a linked list storing the callbacks,
  // where the first node points to the next callback that should be called.
  this._head = null;
  // Maps vnodes to nodes in the linked list.
  this._vnodes = new Map();

  // Build the linked list by iterating through the children in a reverse order
  // (so that _head points to the first node in the end).
  const children = props.children.filter(
    child => child.type.name === Suspense.name
  );
  if (props.revealOrder === "forwards") {
    children.reverse();
  }
  children.some(vnode => {
    this._vnodes.set(vnode, (this._head = { cb: null, next: this._head }));
  });
  return props.children;
};

SuspenseList.prototype.__modifySuspense = function(vnode, cb) {
  const node = this._vnodes.get(vnode);
  this._vnodes.delete(vnode);
  node.cb = cb;

  const order = this.props.revealOrder;
  if (order === "forwards" || order === "backwards") {
    consume(this);
  } else if (order === "together") {
    if (this._vnodes.size === 0) {
      consume(this);
    }
  } else {
    cb();
  }
};

// A helper to walk down and consume the linked list as long as there are resolved callbacks.
const consume = list => {
  for (let node; (node = list._head) && node.cb; list._head = node.next) {
    node.cb();
  }
};

[JoviDeCroock]

Great work already!!!

[JoviDeCroock]

you can save a few bytes by doing .some(t => { t() }) --> this way a return won't be added here

[prateekbh]

why not this._thrillers.forEach(thrill => {thrill.cb()});

[JoviDeCroock]

Wondering if it wouldn't be better to put these in a useEffect since should this happen on every render?

[prateekbh]

I am not sure about this, what if one of the element gets added or removed as a UI logic?

[jviide]

I was looking through the code and got a sneaking suspicion that there might be a hidden assumption: That all <Suspense> elements have a child that suspends. I'm not too familiar with <Suspense>, but there probably are scenarios when this assumption might not hold (?).

Turns out that a test case like this fails (or I messed up the test run somehow 😛):

it('should work even when a <Suspense> child does not suspend', async () => {
	const Component = getSuspendableComponent('A');

	render(
		<SuspenseList revealOrder="forwards">
			<Suspense fallback={<span>Loading...</span>}>
				<div />
			</Suspense>
			<Suspense fallback={<span>Loading...</span>}>
				<Component />
			</Suspense>
		</SuspenseList>,
		scratch
	); // Render initial state

	rerender();
	expect(scratch.innerHTML).to.eql(`<div></div><span>Loading...</span>`);

	await Component.resolve();
	rerender();
	expect(scratch.innerHTML).to.eql(`<div></div><span>A</span>`);
});

[prateekbh]

@jviide the solution in my head is as small as fixing a small Boolean value but in your case we should just log that no promise was thrown, may be you don't really need suspense

[andrewiggins]

Isn't it possible that a component won't suspend until a button is pressed or something? So I wouldn't assume that not suspending on a render is a bug

[prateekbh]

@andrewiggins fixed and it is totally possible to do it. Although wouldn't it be more easier to read if the <suspense> itself is also conditionally rendered?

because as soon as something will throw a promise inside a Suspense all the static things will also get removed for a loader

e.g

<Suspense fallback={<span>Loading...</span>}>
  <ComponentThatWillThrowPromiseLater />
  All the static text present here will also go as soon as the component above will throw the promise...
</Suspense>

[jviide]

@prateekbh @andrewiggins Another scenario that I guess could be common: A component tries to render some remote content that may or may not be already cached locally. If the content is not cached then the render function throws a Promise that resolves when the content is finally fetched. If the content is cached, then the component can render the cached stuff immediately without throwing.

[prateekbh]

Yes. thanks for the explanation. Thats indeed a valid use case.
PR includes your given test case already

[marvinhagemeister]

@jviide I just merged #2106 and it lead to merge conflicts here 😬

[prateekbh]

yep looking at it

[marvinhagemeister]

Sweet🎉♥️