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

Fixed scheduler setTimeout fallback #14358

Merged
merged 3 commits into from
Dec 1, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/jest-mock-scheduler/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# `jest-mock-scheduler`

Jest matchers and utilities for testing the `scheduler` package.

This package is experimental. APIs may change between releases.
8 changes: 8 additions & 0 deletions packages/jest-mock-scheduler/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

export * from './src/JestMockScheduler';
7 changes: 7 additions & 0 deletions packages/jest-mock-scheduler/npm/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

if (process.env.NODE_ENV === 'production') {
module.exports = require('./cjs/jest-mock-scheduler.production.min.js');
} else {
module.exports = require('./cjs/jest-mock-scheduler.development.js');
}
28 changes: 28 additions & 0 deletions packages/jest-mock-scheduler/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"name": "jest-mock-scheduler",
"private": true,
"version": "0.1.0",
"description": "Jest matchers and utilities for testing the scheduler package.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make this private for now until we're sure we want it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

"main": "index.js",
"repository": "facebook/react",
"keywords": [
"jest",
"scheduler"
],
"license": "MIT",
"bugs": {
"url": "https://github.com/facebook/react/issues"
},
"homepage": "https://reactjs.org/",
"peerDependencies": {
"jest": "^23.0.1",
"scheduler": "^0.11.0"
},
"files": [
"LICENSE",
"README.md",
"build-info.json",
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

build-info is metadata managed and used by our release scripts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's in it? Sorry if I missed it somewhere. Do we include it in all packages?

Copy link
Contributor Author

@bvaughn bvaughn Nov 30, 2018

Choose a reason for hiding this comment

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

Just some metadata about this build, e.g.
https://unpkg.com/react@canary/build-info.json

It's useful particularly for the prepare-stable script when it comes to swapping out the shared/ReactVersion value– but also I think it might be generally useful.

"index.js",
"cjs/"
]
}
61 changes: 61 additions & 0 deletions packages/jest-mock-scheduler/src/JestMockScheduler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

// Max 31 bit integer. The max integer size in V8 for 32-bit systems.
// Math.pow(2, 30) - 1
// 0b111111111111111111111111111111
const maxSigned31BitInt = 1073741823;

export function mockRestore() {
delete global._schedMock;
}

let callback = null;
let currentTime = -1;

function flushCallback(didTimeout, ms) {
if (callback !== null) {
let cb = callback;
callback = null;
try {
currentTime = ms;
cb(didTimeout);
} finally {
currentTime = -1;
}
}
}

function requestHostCallback(cb, ms) {
if (currentTime !== -1) {
// Protect against re-entrancy.
setTimeout(requestHostCallback, 0, cb, ms);
} else {
callback = cb;
setTimeout(flushCallback, ms, true, ms);
setTimeout(flushCallback, maxSigned31BitInt, false, maxSigned31BitInt);
}
}

function cancelHostCallback() {
callback = null;
}

function shouldYieldToHost() {
return false;
}

function getCurrentTime() {
return currentTime === -1 ? 0 : currentTime;
}

global._schedMock = [
requestHostCallback,
cancelHostCallback,
shouldYieldToHost,
getCurrentTime,
];
40 changes: 20 additions & 20 deletions packages/scheduler/src/Scheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -447,41 +447,44 @@ var requestHostCallback;
var cancelHostCallback;
var shouldYieldToHost;

if (typeof window !== 'undefined' && window._schedMock) {
var globalValue = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't you mean globalThis? 😏

if (typeof window !== 'undefined') {
globalValue = window;
} else if (typeof global !== 'undefined') {
globalValue = global;
}

if (globalValue && globalValue._schedMock) {
// Dynamic injection, only for testing purposes.
var impl = window._schedMock;
requestHostCallback = impl[0];
cancelHostCallback = impl[1];
shouldYieldToHost = impl[2];
var globalImpl = globalValue._schedMock;
requestHostCallback = globalImpl[0];
cancelHostCallback = globalImpl[1];
shouldYieldToHost = globalImpl[2];
getCurrentTime = globalImpl[3];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a little awkward. I could combine these branches?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's combine.

Copy link
Contributor Author

@bvaughn bvaughn Nov 30, 2018

Choose a reason for hiding this comment

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

Sure, combined in db14ad8

} else if (
// If Scheduler runs in a non-DOM environment, it falls back to a naive
// implementation using setTimeout.
typeof window === 'undefined' ||
// Check if MessageChannel is supported, too.
typeof MessageChannel !== 'function'
) {
// If this accidentally gets imported in a non-browser environment, e.g. JavaScriptCore,
// fallback to a naive implementation.
var _callback = null;
var _currentTime = -1;
var _flushCallback = function(didTimeout, ms) {
var _flushCallback = function(didTimeout) {
if (_callback !== null) {
var cb = _callback;
_callback = null;
try {
_currentTime = ms;
cb(didTimeout);
} finally {
_currentTime = -1;
}
cb(didTimeout);
}
};
requestHostCallback = function(cb, ms) {
if (_currentTime !== -1) {
if (_callback !== null) {
// Protect against re-entrancy.
setTimeout(requestHostCallback, 0, cb, ms);
setTimeout(requestHostCallback, 0, cb);
} else {
_callback = cb;
setTimeout(_flushCallback, ms, true, ms);
setTimeout(_flushCallback, maxSigned31BitInt, false, maxSigned31BitInt);
setTimeout(_flushCallback, 0, false);
}
};
cancelHostCallback = function() {
Expand All @@ -490,9 +493,6 @@ if (typeof window !== 'undefined' && window._schedMock) {
shouldYieldToHost = function() {
return false;
};
getCurrentTime = function() {
return _currentTime === -1 ? 0 : _currentTime;
};
} else {
if (typeof console !== 'undefined') {
// TODO: Remove fb.me link
Expand Down
8 changes: 8 additions & 0 deletions packages/scheduler/src/__tests__/Scheduler-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ describe('Scheduler', () => {
jest.useFakeTimers();
jest.resetModules();

const JestMockScheduler = require('jest-mock-scheduler');
JestMockScheduler.mockRestore();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This convention seemed fairly inline with existing Jest mock packages.


let _flushWork = null;
let isFlushing = false;
let timeoutID = -1;
Expand Down Expand Up @@ -123,16 +126,21 @@ describe('Scheduler', () => {
function shouldYieldToHost() {
return endOfFrame <= currentTime;
}
function getCurrentTime() {
return currentTime;
}

// Override host implementation
delete global.performance;
global.Date.now = () => {
return currentTime;
};

window._schedMock = [
requestHostCallback,
cancelHostCallback,
shouldYieldToHost,
getCurrentTime,
];

const Schedule = require('scheduler');
Expand Down
4 changes: 4 additions & 0 deletions packages/scheduler/src/__tests__/SchedulerDOM-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ describe('SchedulerDOM', () => {
return currentTime;
};
jest.resetModules();

const JestMockScheduler = require('jest-mock-scheduler');
JestMockScheduler.mockRestore();

Scheduler = require('scheduler');
});

Expand Down
4 changes: 4 additions & 0 deletions packages/shared/__tests__/ReactDOMFrameScheduling-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ describe('ReactDOMFrameScheduling', () => {
};
};
jest.resetModules();

const JestMockScheduler = require('jest-mock-scheduler');
JestMockScheduler.mockRestore();

spyOnDevAndProd(console, 'error');
require('react-dom');
expect(console.error.calls.count()).toEqual(1);
Expand Down
2 changes: 2 additions & 0 deletions scripts/jest/setupTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ if (process.env.REACT_CLASS_EQUIVALENCE_TEST) {
toMatchRenderedOutput: JestReact.unstable_toMatchRenderedOutput,
});

require('jest-mock-scheduler');

// We have a Babel transform that inserts guards against infinite loops.
// If a loop runs for too many iterations, we throw an error and set this
// global variable. The global lets us detect an infinite loop even if
Expand Down
9 changes: 9 additions & 0 deletions scripts/rollup/bundles.js
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,15 @@ const bundles = [
externals: ['jest-diff'],
},

/******* Jest Scheduler (experimental) *******/
{
bundleTypes: [NODE_DEV, NODE_PROD, FB_WWW_DEV, FB_WWW_PROD],
moduleType: ISOMORPHIC,
entry: 'jest-mock-scheduler',
global: 'JestMockScheduler',
externals: ['jest-diff'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copypasta

Copy link
Contributor Author

@bvaughn bvaughn Dec 1, 2018

Choose a reason for hiding this comment

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

I assumed this external was required for some reason, since it was listed as a dependency for the jest-react package (even though that package didn't reference it anywhere that I saw).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably copypasta too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh well. We can rip it out in of both then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

},

/******* ESLint Plugin for Hooks (proposal) *******/
{
// TODO: it's awkward to create a bundle for this
Expand Down