Add SuspenseList component#2063
Conversation
1895042 to
6355a67
Compare
…to suspense-list
andrewiggins
left a comment
There was a problem hiding this comment.
Some initial comments. Excited to see this!!
| import { Suspense } from './suspense'; | ||
|
|
||
| // Hook for Suspense boundaries to ask for any extra work before rendering suspended children. | ||
| options.__onSuspensionComplete = (vnode, cb) => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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>There was a problem hiding this comment.
Whoa! Nested SuspenseList 🤯 Gotta think more about that
| * Find and execute all callbacks in order from 2nd position. | ||
| * Breaks as soon as a non resolved(cb===null) suspense found. | ||
| */ | ||
| this._thrillers.find(thrill => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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();
}
};| case 'together': | ||
| this._thrillers.find(thrill => thrill.vnode === vnode).cb = cb; | ||
| if (this._thrillers.every(thriller => thriller.cb)) { | ||
| this._thrillers.forEach(thrill => thrill.cb()); |
There was a problem hiding this comment.
you can save a few bytes by doing .some(t => { t() }) --> this way a return won't be added here
There was a problem hiding this comment.
why not this._thrillers.forEach(thrill => {thrill.cb()});
|
|
||
| SuspenseList.prototype.render = function(props, state) { | ||
| // assuming all suspense woul | ||
| this._thrillers = props.children |
There was a problem hiding this comment.
Wondering if it wouldn't be better to put these in a useEffect since should this happen on every render?
There was a problem hiding this comment.
I am not sure about this, what if one of the element gets added or removed as a UI logic?
…to suspense-list
|
I was looking through the code and got a sneaking suspicion that there might be a hidden assumption: That all 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>`);
}); |
|
@jviide the solution in my head is as small as fixing a small Boolean value but in your case we should just log that |
|
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 |
|
@andrewiggins fixed and it is totally possible to do it. Although wouldn't it be more easier to read if the because as soon as something will throw a promise inside a 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> |
|
@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. |
|
Yes. thanks for the explanation. Thats indeed a valid use case. |
|
yep looking at it |
…to suspense-list
SuspenseListusing above hook to control its children's rendering order.SuspenseList.SuspenseList.SuspenseList.SuspenseList.SuspenseList.SuspenseList.SuspenseList.