Implement pauseExecution, continueExecution, dumpQueue for Scheduler #14053
Conversation
|
React: size: 🔺+1.2%, gzip: 🔺+0.9% Details of bundled changes.Comparing: 98eb5ae...3c6bb9d react
react-dom
react-art
scheduler
Generated by 🚫 dangerJS |
| @@ -31,6 +31,9 @@ var IDLE_PRIORITY = maxSigned31BitInt; | |||
| var firstCallbackNode = null; | |||
|
|
|||
| var currentDidTimeout = false; | |||
| // Pausing the scheduler is useful for debugging. | |||
| var isSchedulerPaused = false; | |||
There was a problem hiding this comment.
@acdlite I think I might want to expose this variable too, since for debugging it's necessary to know if the thing is paused at all in the first place
| updateTestResult(8, `Queue size: ${dumpQueue().length}.`); | ||
| displayTestResult(8); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Thanks for adding a new integration test!
| this, | ||
| arguments | ||
| ); | ||
| } |
There was a problem hiding this comment.
I strongly dislike the fact that the newly proposed API methods only exist on the DEV build.
Do other packages do this? I know that sometimes methods are no-ops in production builds, but having them be missing entirely feels weird.
I also think the fact that these methods exist only for DEV reinforces the notion that you could just use existing browser tooling (like the browser's built-in pause/resume functionality).
There was a problem hiding this comment.
I don't have an opinion on this. no-oping in production seems a good solution in my book.
scripts/rollup/results.json
Outdated
| "size": 95936, | ||
| "gzip": 25258 | ||
| "size": 99473, | ||
| "gzip": 25976 |
There was a problem hiding this comment.
Let's revert the changes to this file.
| @@ -336,6 +347,31 @@ function unstable_scheduleCallback(callback, deprecated_options) { | |||
| return newNode; | |||
| } | |||
|
|
|||
| function unstable_pauseExecution() { | |||
| isSchedulerPaused = true; | |||
There was a problem hiding this comment.
Why isn't the browser's built-in pause/resume functionality sufficient for whatever you're using this for?
| } | ||
|
|
||
| return callbacks; | ||
| } |
There was a problem hiding this comment.
Instead of exposing play, pause, and dump-queue– what if we just exposed the queue itself (firstCallbackNode) somewhere globally in DEV mode only? Then we could just write our own tiny helper function that read from it (if that was actually needed).
(This also assumes that we just use the browser's built-in play/pause script execution functionality.
|
Like @bvaughn I'm unconvinced that
|
|
@bvaughn @acdlite +1 to moving On why pause, some of my thoughts:
If I'm missing something about how built-in tools work or useful workflows do let me know though. How does tooling around scheduler look for you guys? Tagging @n8schloss in case he has more thoughts to add |
Why can't your custom tooling just log to the console? (Why would it need an in-page UI?)
Why would the ability to programmatically pause/unpause the scheduler make this any easier than just setting breakpoints (conditional if needed) in the Source panel and stepping through from there? Aren't you stepping through from a fixed point in code either way? |
It can. Do we not want an in-page UI?
Setting breakpoints doesn't allow me to view the scheduler state after each callback execution, unless I breakpoint in the scheduler itself, which would be unnecessary complicated for most devs. @bvaughn this is why I asked in my original comment, how does tooling for understanding/debugging scheduled code look for you guys? What's your vision here? I want to align goals here. |
This PR is the first I've heard one mentioned. But insomuch as we hope for this package to eventually be adopted by browser vendors natively, I think additions like pause/continue/dump might hurt our chances of adoption. (Presumably if these APIs did become native, native browser DevTools would provide this functionality. In the meanwhile, I think it would be nice if we could get by with existing native tooling– pause/resume script execution, breakpoints, console log, etc.)
I'm not sure I understand the last bit. Most devs probably wouldn't be debugging the scheduler itself, would they? Seems like they would be setting breakpoints in their own (app or framework) code. |
So my thoughts here.
|
Please correct me if I'm wrong. I was under the impression that we essentially were asking them to standardize what's in the "scheduler" package. The
This sounds reasonable. Have we considered creating a custom web extension for this purpose? (I'm not sure how this would work initially. Just thinking out loud.) |
Yes, but my point is that I think there will always be a place for some type of scheduler package even inside of React and it's there that this type of tooling should live. I do think that browsers will want to think about debugging tooling when implementing a scheduler, but that's orthogonal to this.
Cool :) Yeah, I think an open source extension could eventually be neat here too! The extension would still need hooks into the scheduler though, so it doesn't change much for this diff imo. |
Agreed. I think the concern Andrew and I had was that adding non-essential code to this package may hurt our chance for browser adoption.
Well I was asking because if there was a global var (in DEV mode) then perhaps the extension could read from it and provide its own UI to inspect the scheduled state while the scheduler was paused (using the browser's built-in pause script execution functionality). Not sure entirely how this would work, since pausing script execution would also pause posted messages between an extension and the front-end, but...maybe it would be possible somehow. |
Exactly :) Even in that case we're still going to need to let the extension hook into and pause the scheduler. Anyway, I think we should add these functions in for now, and then later if we break some of the internal debugging tools it'll be fine and we'll figure out how to fix them/we're still naming these things |
Why is this necessarily true? I was suggesting that we could play/pause the scheduler using the browser's built-in "pause script execution" functionality. Let's talk about this during the sync tomorrow? This is getting to be too much back and forth. 😄 |
I'm not sure if we can do that and still get the debugger to display what we need and beyond that, it won't let us build the scheduler stepping mechanism that we want to build. So hooks into the scheduler are very important. Anyway, happy to chat about it tomorrow. |
|
Please wrap in a feature flag, then I think this is ok to land. |
…acebook#14053) * Implement pauseExecution, continueExecution, dumpQueue * Expose firstCallbackNode. Fix tests. Revert results.json * Put scheduler pausing behind a feature flag
…acebook#14053) * Implement pauseExecution, continueExecution, dumpQueue * Expose firstCallbackNode. Fix tests. Revert results.json * Put scheduler pausing behind a feature flag
pauseExecution: pauses the scheduler from making progress running subsequent callbacks in the queue.continueExecution: continues executing callbacks from the queue.dumpQueue: returns an array of all scheduled callbacks.Note: at the moment tests don't pass, because
packages/scheduler/src/__tests__/SchedulerUMDBundle-test.internal.jsensures the APIs exported across different modules is consistent, but I only added the functions toumd/scheduler.development. Please advise!cc @n8schloss