Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for multiple callbacks in ReactScheduler #12682

Closed
wants to merge 14 commits into from
Closed

Add support for multiple callbacks in ReactScheduler #12682

wants to merge 14 commits into from

Commits on May 2, 2018

  1. Add support for multiple callbacks in ReactScheduler 

    **what is the change?:**
We want to support calling ReactScheduler multiple times with different
callbacks, even if the initial callback hasn't been called yet.

There are two possible ways ReactScheduler can handle multiple
callbacks, and in one case we know that callbackA depends on callbackB
having been called already. For example;
callbackA -> updates SelectionState in a textArea
callbackB -> processes the deletion of the text currently selected.

We want to ensure that callbackA happens before callbackB. For now we
will flush callbackB as soon as callbackA is added.

In the next commit we'll split this into two methods, which support two
different behaviors here. We will support the usual behavior, which
would defer both callbackA and callbackB.

One issue with this is that we now create a new object to pass to the
callback for every use of the scheduler, while before we reused the same
object and mutated the 'didExpire' before passing it to each new
callback. With multiple callbacks, I think this leads to a risk of
mutating the object which is being used by multiple callbacks.

**why make this change?:**
We want to use this scheduling helper to coordinate between React and
non-React scripts.

**test plan:**
Added and ran a unit test.
    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    d376543 View commit details
    Browse the repository at this point in the history

  2. Move back to using pooling with frameDeadlineObject

    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    fea65d6 View commit details
    Browse the repository at this point in the history

  3. clearTimeoutTimeAndScheduledCallback -> resetScheduledCallback

    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    5f450b8 View commit details
    Browse the repository at this point in the history

  4. Assert callback order in scheduler test for multiple callback support

    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    079bb73 View commit details
    Browse the repository at this point in the history

  5. Fix case of scheduled callbacks which schedule other callbacks 

    **what is the change?:**
We added support for serial scheduled callbacks to schedule more
callbacks, and maintained the order of first-in first-called.

**why make this change?:**
This is sort of a corner case, but it's totally possible and we do
something similar in React.

We wouldn't do this with serial callbacks, but with deferred callbacks
we do a pattern like this:
```
   +                                             +
   |   +--------------------+ +----------------+ | +--------------------------------+ +-------------------------+
   |   |                    | |                | | |                                | |                         |
   |   | main thread blocked| |callback A runs | | | main thread blocked again      | |callback A runs again, finishes
   |   +--------------------+ +-----+----------+ | +--------------------------------+ ++------------------------+
   v                                ^            v                                     ^
schedule +------------------------->+            no time left!+----------------------->+
callbackA                                        reschedule   |
                                                 callbackA    +
                                                 to do more
                                                 work later.

```

**test plan:**
Wrote some fun new tests and ran them~
Also ran existing React unit tests. As of this PR they are still
directly using this module.
    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    df6624b View commit details
    Browse the repository at this point in the history

  6. comment wording nit

    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    c1278be View commit details
    Browse the repository at this point in the history

  7.  remove destructuring

    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    8283c95 View commit details
    Browse the repository at this point in the history

  8. Handle errors thrown by callbacks in schedule 

    **what is the change?:**
We may call a sequence of callbacks in our `schedule` helper, and this
catches any errors and re-throws them later, so we don't get
interrupted or left in an invalid state.

**why make this change?:**
If you have callbacks A, B, and C scheduled, and B throws, we still want
C to get called.

I would prefer if we could find a way to throw and continue syncronously
but for now this approach, of deferring the error, will work.

**test plan:**
Added a unit test.
    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    eb0110f View commit details
    Browse the repository at this point in the history

  9. Add initial test for cIC 

    **what is the change?:**
see title

**why make this change?:**
We are about to add functionality to cIC, where we take an id.
This is in preparation for when we support 'deferred' as well as
'serial' callbacks.

**test plan:**
Run the test.
    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    0c47fe2 View commit details
    Browse the repository at this point in the history

  10. Add stronger types and refactoring in ReactScheduler 

    **what is the change?:**
Some various clean-up changes to pave the way for adding an 'id' for
each callback which can be used to cancel the callback.

**why make this change?:**
To make the next diff easier.

**test plan:**
Ran the tests.
    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    e96d0e3 View commit details
    Browse the repository at this point in the history

  11. Return an id when scheduling callback; use it for cancelling callback 

    **what is the change?:**
See title.

**why make this change?:**
When we support multiple callbacks you will need to use the callback id
to cancel a specific callback.

**test plan:**
Tests were updated, ran all tests.
    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    26b82c8 View commit details
    Browse the repository at this point in the history

  12. Use 'toEqual' instead of 'toBe' in schedule tests; more precise/concise

    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    64b57b1 View commit details
    Browse the repository at this point in the history

  13. Use Map instead of object for registeredCallbackIds

    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    8703f21 View commit details
    Browse the repository at this point in the history

  14. Restructure 'schedule' rIC code for clarity 

    **what is the change?:**
We previously had the following logic:
- pull any old callback out of the 'scheduledCallback' variable
- put the new callback into 'scheduledCallback'
- call the old callback if we had found one.

Splitting out the logic for handling the old callback into two blocks
was hard to read, so we restructured as so:
- if no old callback was found, just put the new callback into the
'scheduledCallback' variable.
- Otherwise, if we do find an old callback, then:
  - pull any old callback out of the 'scheduledCallback' variable
  - put the new callback into 'scheduledCallback'
  - call the old callback

**why make this change?:**
Code clarity

**test plan:**
Ran the tests
    @flarnie
    flarnie committed May 2, 2018
    Configuration menu
    Copy the full SHA
    5ba093b View commit details
    Browse the repository at this point in the history