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.

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

Quick fix for minor typo in ReactScheduler #12834

Merged
merged 1 commit into from
May 16, 2018

Conversation

flarnie
Copy link
Contributor

@flarnie flarnie commented May 16, 2018

what is the change?:
We were setting a flag after some early returns, should have set it
right away.

To be fair, it's not clear how you can hit a problem with the current
state of things. Even if a callback is cancelled, it's still in the
'pendingCallbacks' queue until the rAF runs, and we only schedule a rAF
when there are pendingCallbacks in the queue.

But since this is obviously wrong, going to fix it.

We will be adding a regression test in a follow-up PR.

why make this change?:
To fix a random bug which was popping up.

test plan:
Adding a regression unit test in a follow-up PR.

**what is the change?:**
We were setting a flag after some early returns, should have set it
right away.

To be fair, it's not clear how you can hit a problem with the current
state of things. Even if a callback is cancelled, it's still in the
'pendingCallbacks' queue until the rAF runs, and we only schedule a rAF
when there are pendingCallbacks in the queue.

But since this is obviously wrong, going to fix it.

We will be adding a regression test in a follow-up PR.

**why make this change?:**
To fix a random bug which was popping up.

**test plan:**
Adding a regression unit test in a follow-up PR.
@flarnie flarnie requested a review from acdlite May 16, 2018 20:50
@flarnie
Copy link
Contributor Author

flarnie commented May 16, 2018

Thanks @rhagigi for spotting this.

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make sure we write a test for this once the fix is landed.

@flarnie flarnie merged commit 2da155a into facebook:master May 16, 2018
flarnie pushed a commit that referenced this pull request May 18, 2018
)

* Scheduler red->green unit test for bug

* fix lint issue

* ran prettier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants