Introduce Suspense and lazy #1593

sventschui · 2019-05-02T16:34:31Z

This introduces Suspense and lazy. There are some open things tough.

EDIT: After the excellent feedback on this PR things seem to be close to finish 🎉 Currently this PR adds 187 bytes gzipped to core.

Open things to do / disucss

Fix issue of Suspense inside Fragment messing up the order of the DOM (Fix incorrect DOM when Fragment has siblings #1605)
- Skipping this test for now
Should Suspense / lazy remain part of the core or live in preact/suspense or preact/compat? This would save 137 bytes from core what would only add 50 bytes to core for somebody not using suspense
- Keeping them in core for now. If you wish I can move them...
Shall we drop isSuspense = true inside catchErrorInComponent resulting in 3B saving but also make errors thrown inside _childDidSuspend result in a Missing Suspense error?
- I'd not do so, marking as complete
Warn about propTypes on lazy() in preact/debug
Warn about missing Suspense in preact/debug
- Not possible without some more changes. A Missing Suspense error must do the job for now
Golf some bytes
Create a demo

JoviDeCroock

What if there's an ErrorBoundary between the lazy and Suspense component. Wanted to implement it like this aswell but that's the issue I kept bumping against.

Ideally those warnings are thrown in /debug but can be discussed later.
As to the testing strategy I'm not sure about that

sventschui · 2019-05-02T16:39:41Z

We could add a prop (e.g. promise.Suspense = Symbol.for('Suspense')) to the promises thrown by lazy and then skip user-land componentDidCatch for these errors

We should also investigate how React behaves as Suspense is catching promises in React too.

EDIT: Just hacked together a fiddle to check the behaviour of React. Seems React doesn't send promises to user-land componentDidCatch at all. Even when throwing them in custom components. What React does is throwing an error if there is no Suspense in the tree above a component throwing a Promise: https://jsfiddle.net/sventschui/dovLt0em/6/

I'd propse to not send anything that has a then function to componentDidCatch. What is missing then is a way to throw an error if there is no suspense in the hierarchy. Maybe you can point me into the right direction to implement this?

coveralls · 2019-05-02T16:40:24Z

Coverage remained the same at 100.0% when pulling 0431636 on sventschui:feature/suspense-lazy into b1a1956 on developit:master.

sventschui · 2019-05-02T21:46:51Z

While writing the tests one thing came to my attention. When using Suspense in React the suspended component tree doesn't loose state. It isn't unmounted, just "parked". I'm not sure whether this is possible with the current solution. Do you have an idea how to handle this?

EDIT: Seems I was wrong on this. Components are re-created in react too (see https://jsfiddle.net/sventschui/3auzjo67/21/)

JoviDeCroock · 2019-05-02T21:54:29Z

That would be something very specific to a two phase render cycle and is not possible at this time as far as I know.

I’m also not sure as to what point we want to support this in core since this adds a lot of bytes, it’s already adding 400 at this point.

Cc @developit @marvinhagemeister @andrewiggins

sventschui · 2019-05-03T05:55:30Z

I guess most of the bytes come from a huge error message I copied from React. We should replace this with a shorter one or even with just a short-link to a wiki/error description page. Also we might not want to bundle in the Suspense and lazy components but rather have them in like preact/suspense to save this bytes for folks not using Suspense. Do you have a strategy in place for things like this?

The state keeping thing is quite important as it breaks all the tests and compat to React currently.

When not having suspense in core we at least should try to make it possible in a user-land extension. That means having catchErrorInComponent extendable/replaceable and something to make the state keeping possible. Honestly the dirties hack that comes to my mind here is keeping it as display hidden in user-land code but that will definitely introduce other issues.

For the state keeping we might want to try to flag the direct children of Suspend as suspended, keep them as a prop to Suspense and not call unmount on these. During the next render simply re-run render on these children. Thinking about this makes me think of implementing Suspense not as a component but as code in the core that gets triggered if a component is of type Symbol.for("Suspense")

Sorry for my thought flush on my phone. Will look into this a bit deeper today once I get my fingers to my macbook.

EDIT: React doesn't seem to keep state neither. I was wrong on this. (see https://jsfiddle.net/sventschui/3auzjo67/21/)

marvinhagemeister · 2019-05-03T06:25:51Z

src/diff/index.js

+
+	if (isSuspend && suspendingComponent) {
+		// TODO: what is the preact way to determine the component name?
+		const componentName = suspendingComponent.displayName || (suspendingComponent._vnode && suspendingComponent._vnode.type


Maybe we can move this section into preact/debug. A Suspense component without a fallback doesn't seem to make much sense, so we could just always warn on that. This would also keep core small and tiny 🎉

EDIT: Removing that would save 158 B 👍

This also allows the recheck of isSuspsend and suspendingComponent, so that will save a lot

I still throw the Missing Suspense error on the suspendingComponent.

Would you want to save some bytes and just throw the error at the end of the function instead of the suspendingComponent?

What he's saying is to throw it in a hook from preact/debug since it doesn't really matter in production anyway. I also think that is the best approach to maintain our lightweight profile

Can you guys give me a hint on where I could add this to preact/debug. I did not find a proper place to add it

test/browser/suspense.test.js

src/suspense.js

src/diff/index.js

marvinhagemeister

This is a really cool start 👍 Left a few comments, let me know what you think 🙂

sventschui · 2019-05-03T07:55:48Z

Thanks for these hints. Will definitely incorporate these once I get closer to a working implementation.

The thing that currently bothers me is keeping the state of the suspended component tree (as React does). I was thinking of doing the following:

Not unmount (but remove DOM of) children of Suspense (here https://github.com/developit/preact/blob/master/src/diff/index.js#L29)
Re-render the previous children of suspense here https://github.com/developit/preact/blob/master/src/diff/index.js#L143

Can't think of how to achieve the second point. One of you guys got an idea how to solve this?

EDIT: It seems I was wrong. React doesn't seem to keep state neither (see https://jsfiddle.net/sventschui/3auzjo67/21/)

sventschui · 2019-05-03T16:15:44Z

I justed hacked a working solution together to keep state of suspended components 🎉 .

Would be cool if you could look into this. I don't really feel confident with the current solution. I'll invest some more time when I get to it.

JoviDeCroock

First and forall, congratulations on the already -100B 💯

Looking like an awesome feature to have in all honesty.
Something that I'm thinking about is that it could actually flicker if we don't park DOM (we can test this in demo maybe)

ForsakenHarmony · 2019-05-03T23:06:44Z

might want to have a test with multiple suspended components in one suspense

src/diff/index.js

LukasBombach · 2019-05-06T15:30:31Z

@sventschui @marvinhagemeister can you check if this implementation works with Next?

I have setup a monorepo project that uses this branch with next and a next plugin similar to @zeit/next-preact. Basically I have copy and pasted their code and used it with this branch. Check it out here:

https://github.com/spring-media/preact-next-suspense

I have written a bit of a documentation to explain the repo in the README

sventschui · 2019-05-06T16:39:08Z

@LukasBombach as mentioned in slack: The error is due to an old version of preact-render-to-string. You need to update to >=5

developit · 2019-05-06T17:55:56Z

src/diff/index.js

+						component._childDidSuspend(error);
+					}
+					else {
+						continue;


Maybe move to here?

Suggested change

continue;

return catchErrorInComponent(Error('Missing Suspense'), suspendingComponent);

The continue results in traversing up the tree. This would break the tree traversing AFAIK

developit · 2019-05-06T17:58:15Z

Question: what's the reason for using ._childDidSuspend? It seems like the Suspense component could just implement componentDidCatch:

class Suspense {
  componentDidCatch(e) {
    if (e && e.then) e.then(() => this.setState({}));
    else throw e;
  }
}

This would mean that folks don't pay for Suspense implementation details if they don't import Suspense.

sventschui · 2019-05-06T18:07:26Z

Well the naming of the method is irrelevant. It just needs to take precedence over regular componentDidCatch when the error thrown is a promise (even if between the throwing component and <Suspense> is another error boundary).

Edit: Did not yet check but if renaming saves bytes then I'll rename it

LukasBombach · 2019-05-06T21:51:30Z

@LukasBombach as mentioned in slack: The error is due to an old version of preact-render-to-string. You need to update to >=5

👌

marvinhagemeister · 2019-05-07T06:41:24Z

src/diff/index.js

-			diffChildren(parentDom, newVNode, oldVNode, context, isSvg, excessDomChildren, mounts, c, oldDom);
+			// Passing the ancestorComponent instead of c here is needed for catchErrorInComponent
+			// to properly traverse upwards through fragments to find a parent Suspense
+			diffChildren(parentDom, newVNode, oldVNode, context, isSvg, excessDomChildren, mounts, ancestorComponent, oldDom);


Good catch! This may also fix an issue I was having when working on the devtools where Fragments would break an upwards traversal 👍

marvinhagemeister

Love the journey of this PR 🎉 In my opinion it is ready to be merged. I'd just skip the test case with Fragments (via it.skip()) because this problem will be dealt with separately in #1605 🎉

JoviDeCroock

As stated by @marvinhagemeister this PR is amazing! Great work on it, just one thing that needs to be added is a test that tests your error message in /debug then the coverall will report 100% again.

LukasBombach · 2019-05-07T08:54:38Z

@sventschui @marvinhagemeister @JoviDeCroock I can confirm this PR works with Next 8.x using preact-render-to-string@next as @sventschui proposed:

spring-media/preact-next-suspense@5a7de52

38elements · 2019-05-07T12:10:57Z

src/diff/index.js

 			}
 		}
 	}
+
+	// TODO: Add a react-like error message to preact/debug


Does this Todo need to be resolved ?
If it is unnecessary, I think it is better to remove it.

I'd like to tackle this (see TODO in PR description) but currently don't know how to do this without adding too many bytes to core

LukasBombach · 2019-05-07T13:01:28Z

Oh wait, there is still an issue with this and Next.js when you run my repo and open http://localhost:3000, you'll get an error in node saying

[ error ] TypeError: component.props.handleStateChange is not a function
    at emitChange (preact-next-suspense/node_modules/next-server/dist/lib/side-effect.js:11:29)
    at new SideEffect (preact-next-suspense/node_modules/next-server/dist/lib/side-effect.js:26:17)
    at s (preact-next-suspense/node_modules/preact-render-to-string/dist/index.js:1:1371)
    at s (preact-next-suspense/node_modules/preact-render-to-string/dist/index.js:1:1683)
    at s (preact-next-suspense/node_modules/preact-render-to-string/dist/index.js:1:1683)
    at s (preact-next-suspense/node_modules/preact-render-to-string/dist/index.js:1:1683)
    at s (preact-next-suspense/node_modules/preact-render-to-string/dist/index.js:1:2849)
    at s (preact-next-suspense/node_modules/preact-render-to-string/dist/index.js:1:1683)
    at s (preact-next-suspense/node_modules/preact-render-to-string/dist/index.js:1:1683)
    at s (preact-next-suspense/node_modules/preact-render-to-string/dist/index.js:1:1683)
    at s (preact-next-suspense/node_modules/preact-render-to-string/dist/index.js:1:1683)
    at s (preact-next-suspense/node_modules/preact-render-to-string/dist/index.js:1:1683)
    at s (preact-next-suspense/node_modules/preact-render-to-string/dist/index.js:1:1683)
    at s (preact-next-suspense/node_modules/preact-render-to-string/dist/index.js:1:1683)
    at render (preact-next-suspense/node_modules/next-server/dist/server/render.js:86:16)
    at renderPage (preact-next-suspense/node_modules/next-server/dist/server/render.js:211:20)

sventschui · 2019-05-07T13:48:46Z

@LukasBombach I once encountered this issue too. As far as I remember it was caused by context not being set correctly but this should be fixed. I can run your example without any issues

marvinhagemeister · 2019-05-07T14:00:34Z

@LukasBombach a new version for preact-render-to-string with support for creeateContext hasn't been published yet. The code is already in master but I was too busy the past days. The next release should be up by the end of the week.

EDIT: published it 🎉

LukasBombach · 2019-05-07T14:08:04Z

@sventschui have you checked your node console after loading the index page in your browser?

LukasBombach · 2019-05-08T09:37:13Z

EDIT: published it 🎉

@marvinhagemeister @sventschui that update seems to have fixed the error 🎉

PS: I have updated my repo for you to try out

debug/src/debug.js

src/suspense.js

marvinhagemeister

Wohoo! Let's do this👍🎉✔️

LukasBombach · 2019-05-09T08:27:46Z

🎉 😍

Introduce first draft of Suspense and lazy

5b28ba5

JoviDeCroock reviewed May 2, 2019

View reviewed changes

Sven Tschui added 2 commits May 2, 2019 23:14

Add tests, handle missing suspense, handle custom error boundaries

1d525f1

Work on sunspense...

eb9c7db

Merge branch 'master' into feature/suspense-lazy

20cb0ae