-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds requestIdleCallback to vdom/component #409
Conversation
6x is such a nice win. Great work, @paullewis 🎉 Shipping it as an opt-in (at least for now) should give folks a chance to try it out for a while in the wild and let you know if anything breaks. |
component.componentWillReceiveProps(props, context); | ||
} | ||
const cancelIdleCallback = | ||
requestIdleCallback !== 'null' ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'null'
? Did you mean null
?
requestIdleCallback !== null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, I didn't. I did a typeof test earlier on, and that carried through. Awwwwkward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paullewis you did return window.requestIdleCallback
to const requestIdleCallback
if typeof window !== 'undefined' && typeof window.requestIdleCallback !== 'undefined'
. So, now requestIdleCallback
contains a function or null
. Not a typeof
from earlier test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MakingOff is right, requestIdleCallback
will be null
not 'null'
when window.requestIdleCallback
is not defined, so cancelIdleCallback
will be undefined
instead of null
in that case.
It doesn't actually cause an error since cancelIdleCallback
is never called when requestIdleCallback
is not defined, but still worth fixing 😄
Looking into some breakages from this, sorry for the merge wait! |
I like the idea but wonder if this should just be a component and not part of the framework. Roughly: Instead of:
it would be:
Rough implementation:
(this would need a lot more to work in real life but hopefully conveys the idea). This feels more idiomatic and wouldn't require breaking changes or an expansion of the API. |
One thing I like about @ainth's idea is that it allows for the async rendering to be contextual. I could see having different variations of AsyncRenderer for different strategies. For example, I could see an InViewRenderer that waits until the component is within some threshold of being in the viewport before rendering. How would you do stuff like that using the |
We have a Not compatible with SSR though, since it uses I think that's the most interesting part of Paul's implementation here: you can render on the server, but then just slowly hydrate as needed on the client. I think this is a really interesting space. |
Also: for people who are as interested as I am with this - here's a really interesting demo of Paul's amended version of Preact asynchronously rendering a pythagoras tree. You can see the effect of the fully async rendering as the tree depth grows beyond what your CPU can render in in a single pass - it starts to render progressively based on available time. |
@developit That tree demo is mesmerising! I love where this is going 👍 |
This demo looks very similar to the Sierpinski Triangle Demo that we've been using to test React Fiber's scheduling. The difference there is that we can hold the rendering to the screen until everything is complete. We do have a mode that demos the progressive work it does but we only use that to demo the work that is done behind the scenes - not user visible. In most cases you don't actually want to show the progression of too many incomplete states to the user. For example on Facebook we've actively been trying to get rid of that since the load experience is jarring when there are too many small visible updates to the screen. So instead of showing them immediately we delay them for later. In the mean time we can still commit smaller updates elsewhere if they're higher priority for responsiveness so nothing blocks. |
@sebmarkbage I believe this demo code was actually @Swizec's that I punked to test out this rendering mode. I do have a preact version of that triangle demo floating around somewhere though, it shows how the current async rendering works prior to Paul's PR here - not supporting async sibling rendering. Your point about perceived lag and meaningful scheduling is the thing that's making me think this PR through perhaps a little more thoroughly than I ought to. I don't think there's a one-size-fits-all scheduler, so I want to provide a clean extension point so that people can build their own schedulers and publish them. There are many use-cases, so I foresee many scheduling techniques to match. |
@developit Certain scheduling techniques are anti-modular. Those are the ones that need built-in support. Anything else can be user space though. |
This is very important: How would this for example work with the microtask or rAF batching? It would be very hard for users to compose/control strategies with a Instead, I think it would be better for the framework to expose primitives to the user, for example like async render(node, data){
const { pause } = framework
// render some things
await pause()
// render some things
await pause()
// render some things
} Just awaiting on a noop would push the component to the back of the render queue. You could have another one (deopt?) that would use rIC that would essentially make it low priority by only continuing rendering when other components have rendered (even if they joined the queue after the component was paused). You could potentially allow passing a value to indicate different priority lanes for rendering components. And a timeout. You could pause until visible, but perhaps first just fixing width/height though so the container has the right scrollbar length etc etc. Furthermore, these common cases could also be used by render middleware by decorating the component rather than from inside. This extension point allows more innovation to take place outside the framework core. The key change to allow progressive rendering is that the render function needs to be The main objection I've heard against this is that it's no longer "pure". What's important with that is that components are still declarative and a function of data, it just doesn't have to resolve immediately but can over time. The overall render process is fundamentally async. In most cases you have to wait for data to load. With the above, you can just do Forcing the tree of all components to be sync causes a lot of awkwardness atm for UI developers: it either pushes developers to try ideologically load everything at the root (doesn't scale), or splitting into an arbitrary async phase and sync phase with more lifecycle (i.e. Related: |
Wouldn't a generator be more appropriate than an async function for modelling this? For what it's worth, making render asynchronous does not solve Paul's issue on its own. If you render 500 components in a common parent (as this demo does), each is not sufficiently aware of the other or its place in the try that it could determine an appropriate priority for itself. Centralizing that decision into a hook makes this possible, since all component render requests flow through a single point that can use as much state as it needs to implement an appropriate algorithm for prioritizing updates. |
@developit what is this |
@swernerx I like this so much! https://github.com/developit/preact-virtual-list |
Yeah @luisvinicius167's reference is the closest I have publicly available. The other one is internal. I'll ask about open-sourcing it |
Wow ! You may also want to have a look at https://github.com/Synchronized-TV/react-async-child (the react-router 4 example there is interesting) |
@jide That's similar to what I had originally suggested (AsyncComponent). However, Paul pointed out that it breaks when using Server-Side Rendering. The statically rendered HTML element children of the lazily-rendered component get removed. The hope here is to work around that by not only lazily rendering children, but the component itself. |
Ah ok, I better understand the issue ! |
@developit Any new plans yet regarding this PR? :) |
By the way, as React Suspense is on the way. Might this PR be the beginning of Preact Suspense? 🤔 |
Hey is this still on the table? 🙂 Are you guys considering it? |
I'm always considering it... /me stares at @developit 😁 |
Will this PR be merged in preact X? This seems to be another way to implement time slicing! |
@132yse Sorry for the late response. I missed it in the pool of notifications :/ Time-Slicing refers to the ability to schedule updates with different priorities. This is a huge change and requires a 2-phase commit. It's something we may set our sights on for 2019, but nothing concrete has been decided yet. The changes in this PR debounce updates via |
Friendly ping for this PR, it's been a year. Any plans, anything non-maintainers could help with getting this PR merged or improved? |
hey @kurtextrem This is a PR for Preact 8, if you're using Preact X you can use this in user-land. We don't want to impose this increase in size for everyone who potentially won't need it.
|
Thanks for the update! 👍 409 will always live on in my heart. |
As it will in our hearts as well, @paullewis. 🙇 |
So from our offline conversation, I've been working on adding
requestIdleCallback
toComponent
. This is a straw man approach, so I'm not going to be all hurt if you're like "ick, no, don't do THAT!" or anything 😄What this does is wraps the
renderComponent
function inners inrequestIdleCallback
if the component in question is set to render as async:If the component is pending reconciliation and a second call to
renderComponent
is made for it, it cancels the first and replaces it with the most recent.When faced with a decent number of components to boot from SSR, I'm seeing the main thread blocking time on my Nexus 5X go from ~1 second:
All the way down to ~160ms:
Obviously this could be a breaking change, though given it's an opt-in behaviour I hope it wouldn't really sting anyone!
I mean, who doesn't enjoy a 6x reduction in blocking main thread time? 🎉 🎉 🎉 🎉