[schedule 1/n] Support cancelling scheduled callback with an id #12743
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**what is the change?:** see title **why make this change?:** Once we support multiple callbacks you will need to use the id to specify which callback you mean. **test plan:** Added a test, ran all tests, lint, etc.
flarnie
commented
May 4, 2018
| // Avoid using 'catch' to keep errors easy to debug | ||
| } finally { | ||
| // always clean up the callbackId, even if the callback throws | ||
| delete registeredCallbackIds[callbackId]; |
There was a problem hiding this comment.
We will later refactor this for error handling, but for now this works.
|
ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.3% Details of bundled changes.Comparing: 25dda90...be7254b react-dom
react-art
react-scheduler
Generated by 🚫 dangerJS |
flarnie
changed the title
|
|
bvaughn
reviewed
May 8, 2018
| let timeoutTime = -1; | ||
| // Number.MAX_SAFE_INTEGER is not supported in IE | ||
| const MAX_SAFE_INTEGER = Number.MAX_SAFE_INTEGER || 9007199254740991; | ||
| let callbackIdCounter = 1; |
| if (options != null && typeof options.timeout === 'number') { | ||
| timeoutTime = now() + options.timeout; | ||
| } | ||
| // This assumes that we only schedule one callback at a time because that's | ||
| // how Fiber uses it. | ||
| const latestCallbackId = getCallbackId(); |
There was a problem hiding this comment.
nit: Why "latest"? Isn't it actually a "new" id?
| callback(frameDeadlineObject); | ||
| } | ||
| scheduledCallbackConfig = null; | ||
| safelyCallScheduledCallback(callback, callbackId); |
There was a problem hiding this comment.
Does this need to be a separate function call? I guess it could get inlined anyway so it probably doesn't matter.
There was a problem hiding this comment.
This does eventually get called in multiple places, and will be more complex once we implement error handling.
| if (options != null && typeof options.timeout === 'number') { | ||
| timeoutTime = now() + options.timeout; | ||
| } | ||
| // This assumes that we only schedule one callback at a time because that's | ||
| // how Fiber uses it. |
There was a problem hiding this comment.
I think this comment should be above scheduledCallbackConfig = {...} rather than the call to getCallbackId.
But also, the comment confuses me a little. A lot of this module seems written to support multiple, concurrent callbacks (like the fact that rIC returns an id and adds to a registeredCallbackIds map).
There was a problem hiding this comment.
It is laying the groundwork for support to multiple callbacks, yes. I have a follow-up PR that implements that support, just breaking things into multiple steps.
There was a problem hiding this comment.
Regarding the comment - it's outdated, will remove either in this PR or the next.
acdlite
reviewed
May 9, 2018
| let isIdleScheduled = false; | ||
| let timeoutTime = -1; | ||
| // Number.MAX_SAFE_INTEGER is not supported in IE | ||
| const MAX_SAFE_INTEGER = Number.MAX_SAFE_INTEGER || 9007199254740991; |
There was a problem hiding this comment.
I don't think we need to account for the counter exceeding the maximum integer :D
There was a problem hiding this comment.
Will remove, thanks!
There was a problem hiding this comment.
That’s a century in JavaScript Library Years.
|
Will fix the commented items in the follow-up PR, and since this is a subset of that PR closing this one in favor of fixing and merging #12746 |
flarnie
added a commit
to flarnie/react
that referenced
this pull request
May 9, 2018
See comments on facebook#12743
flarnie
added a commit
that referenced
this pull request
May 9, 2018
* Support using id to cancel scheduled callback **what is the change?:** see title **why make this change?:** Once we support multiple callbacks you will need to use the id to specify which callback you mean. **test plan:** Added a test, ran all tests, lint, etc. * ran prettier * fix lint * Use object for storing callback info in scheduler * Wrap initial test in a describe block * Support multiple callbacks in `ReactScheduler` **what is the change?:** We keep a queue of callbacks instead of just one at a time, and call them in order first by their timeoutTime and then by the order which they were scheduled in. **why make this change?:** We plan on using this module to coordinate JS outside of React, so we will need to schedule more than one callback at a time. **test plan:** Added a boatload of shiny new tests. :) Plus ran all the old ones. NOTE: The tests do not yet cover the vital logic of callbacks timing out, and later commits will add the missing test coverage. * Heuristic to avoid looking for timed out callbacks when none timed out **what is the change?:** Tracks the current soonest timeOut time for all scheduled callbacks. **why make this change?:** We were checking every scheduled callback to see if it timed out on every tick. It's more efficient to skip that O(n) check if we know that none have timed out. **test plan:** Ran existing tests. Will write new tests to cover timeout behavior in more detail soon. * Put multiple callback support under a disabled feature flag **what is the change?:** See title **why make this change?:** We don't have error handling in place yet, so should maintain the old behavior until that is in place. But want to get this far to continue making incremental changes. **test plan:** Updated and ran tests. * Hide support for multiple callbacks under a feature flag **what is the change?:** see title **why make this change?:** We haven't added error handling yet, so should not expose this feature. **test plan:** Ran all tests, temporarily split out the tests for multiple callbacks into separate file. Will recombine once we remove the flag. * Fix nits from code review See comments on #12743 * update checklist in comments * Remove nested loop which calls additional timed out callbacks **what is the change?:** We used to re-run any callbacks which time out whilst other callbacks are running, but now we will only check once for timed out callbacks then then run them. **why make this change?:** To simplify the code and the behavior of this module. **test plan:** Ran all existing tests. * Remove feature flag **what is the change?:** see title **why make this change?:** Because only React is using this, and it sounds like async. rendering won't hit any different behavior due to these changes. **test plan:** Existing tests pass, and this allowed us to recombine all tests to run in both 'test' and 'test-build' modes. * remove outdated file * fix typo
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
what is the change?:
-> Stores an id for each callback, removes it after callback is cancelled or called.
-> We don't use 'Map' because this library is intended for use outside of React, don't want to add dependency on a polyfill if we can avoid it.
-> We may eventually just use a linked list, but I still like the simplicity of being able to cancel the callback in O(1) time using some kind of key/value storage with the id.
-> Since we have a callback, timeout, and id which are always used together, makes sense to store them in an object. Also once we store multiple callbacks will need to create objects to store these pieces of data for each. If we really want to avoid creating objects, could use arrays with data for a given callback at the same index, or we could use object pooling. Would explore that in a later PR, seems like a premature optimization though.
why make this change?:
Once we support multiple callbacks you will need to use the id to
specify which callback you mean.
Also storing the callback data in an object sets us up nicely for storing an array of pending callback configs.
test plan:
Added a new test for
cICand ran all tests.