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

[scheduler] Improve naive fallback version used in non-DOM environments #13740

Merged
merged 1 commit into from Sep 27, 2018

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Sep 27, 2018

Added some tests for the non-DOM version of Scheduler that is used as a fallback, e.g. Jest. The tests use Jest's fake timers API:

  • jest.runAllTimers() flushes all scheduled work.
  • jest.advanceTimersByTime(ms) flushes only callbacks that expire within the given milliseconds.

These capabilities should be sufficient for most product tests. Because jest's fake timers do not override performance.now or Date.now, we assume time is constant. This means Scheduler's internal time will not be aligned with other code that reads from performance.now. For finer control, the user can override window._sched like we do in our tests. We will likely publish a Jest package that has this built in.

Added some tests for the non-DOM version of Scheduler that is used
as a fallback, e.g. Jest. The tests use Jest's fake timers API:

- `jest.runAllTimers(ms)` flushes all scheduled work, as expected
- `jest.advanceTimersByTime(ms)` flushes only callbacks that expire
within the given milliseconds.

These capabilities should be sufficient for most product tests. Because
jest's fake timers do not override performance.now or Date.now, we
assume time is constant. This means Scheduler's internal time will not
be aligned with other code that reads from `performance.now`. For finer
control, the user can override `window._sched` like we do in our tests.
We will likely publish a Jest package that has this built in.
@sizebot
Copy link

sizebot commented Sep 27, 2018

Fails
🚫

node` failed.

Log

Error:  { FetchError: invalid json response body at http://react.zpao.com/builds/master/_commits/4d17c3f0516ddd5a7d23a460abbc3c3ddeab98a9/results.json reason: Unexpected token < in JSON at position 0
    at /home/circleci/project/node_modules/node-fetch/lib/body.js:48:31
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:189:7)
  name: 'FetchError',
  message: 'invalid json response body at http://react.zpao.com/builds/master/_commits/4d17c3f0516ddd5a7d23a460abbc3c3ddeab98a9/results.json reason: Unexpected token < in JSON at position 0',
  type: 'invalid-json' }

Generated by 🚫 dangerJS

}
requestHostCallback = function(cb, ms) {
if (_currentTime !== -1) {
// Protect against re-entrancy. Jest's fake timer queue flushes timer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe remove mentions of Jest? If it’s meant to be generic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point

Copy link
Collaborator

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

This looks okay to me.

InteractivePriority = Scheduler.unstable_InteractivePriority;
});

it('runAllTimers only flushes some callbacks', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Ambiguous wording

// purposes, like with jest's fake timer API.
var _callback = null;
var _currentTime = -1;
function flushCallback(didTimeout, ms) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function declaration fails the no-inner-declarations lint rule.

@acdlite acdlite merged commit 4d17c3f into facebook:master Sep 27, 2018
@gaearon gaearon mentioned this pull request Oct 23, 2018
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

5 participants