Fixed scheduler setTimeout fallback #14358

bvaughn · 2018-11-29T23:57:58Z

Resolves #14352 (and facebook/react-native/issues/21967)

Restores a working setTimeout based fallback for the scheduler package.
Relocates test-specific code into a new NPM package, jest-mock-scheduler.
Updates tests.

I tested and verified that these updates to scheduler fix the reported React Native useEffects bug.

I've also published a 0.0.0 release to jest-mock-scheduler to reserve the name.

Moved unit-test-specific setTimeout code into a new NPM package, jest-mock-scheduler.

bvaughn · 2018-11-29T23:58:31Z

packages/scheduler/src/Scheduler.js

+  requestHostCallback = globalImpl[0];
+  cancelHostCallback = globalImpl[1];
+  shouldYieldToHost = globalImpl[2];
+  getCurrentTime = globalImpl[3];


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

Let's combine.

Sure, combined in db14ad8

bvaughn · 2018-11-29T23:59:12Z

packages/scheduler/src/__tests__/Scheduler-test.js

@@ -30,6 +30,9 @@ describe('Scheduler', () => {
    jest.useFakeTimers();
    jest.resetModules();

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


This convention seemed fairly inline with existing Jest mock packages.

sizebot · 2018-11-30T00:06:09Z

Details of bundled changes.

Comparing: 88ada98...2524836

jest-mock-scheduler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
jest-mock-scheduler.development.js	n/a	n/a	0 B	1.51 KB	0 B	732 B	NODE_DEV
jest-mock-scheduler.production.min.js	n/a	n/a	0 B	680 B	0 B	445 B	NODE_PROD
JestMockScheduler-dev.js	n/a	n/a	0 B	1.48 KB	0 B	711 B	FB_WWW_DEV
JestMockScheduler-prod.js	n/a	n/a	0 B	1.06 KB	0 B	532 B	FB_WWW_PROD

Generated by 🚫 dangerJS

gaearon · 2018-11-30T01:28:05Z

Can you tell more about the intended audience of jest-mock-scheduler? Who would need to test the scheduler itself? Or is it about testing code that relies on scheduler? Could that be used to trigger useEffect callbacks from tests?

bvaughn · 2018-11-30T02:46:01Z

Can you tell more about the intended audience of jest-mock-scheduler?

For the moment– we are the audience (React team and a few Facebook engineers working in www).

It makes it more convenient to test updates at a finer grain (advancing Jest timers a little bit, verify some things, then a little bit more). Seemed like the easiest path forward to replacing the setTimeout branch of scheduler with a functional fallback without breaking existing tests.

gaearon · 2018-11-30T02:48:50Z

I mean more like, do you expect product developers to ever need it? Does it help testing things like passive effects?

bvaughn · 2018-11-30T02:58:02Z

I'm not sure yet how useful it will end up being. @acdlite has thought about this before. Maybe he has a better intuition.

gaearon · 2018-11-30T15:24:59Z

Can we fix the bug without introducing a new package? My concern is just that we already created a package (jest-react) but it's not clear who or when will use it. This adds one more, and I still don't understand who they're for. It seems like overkill if we add public packages which we only use ourselves. But maybe I'm missing something. If this is meant for public consumption, maybe it would make sense to unify jest-react and jest-mock-scheduler somehow. Or even re-export all of it from react-test-renderer/something.

bvaughn · 2018-11-30T15:39:47Z

@acdlite suggested a new package initially, so I'd like for him to weigh in here.

The part of this PR I'm most concerned about is fixing the setTimeout fork of scheduler to unbreak useEffect for React Native 😄

gaearon · 2018-11-30T18:42:16Z

packages/jest-mock-scheduler/package.json

+  "files": [
+    "LICENSE",
+    "README.md",
+    "build-info.json",


What is this?

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

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

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.

acdlite · 2018-11-30T23:17:10Z

packages/jest-mock-scheduler/package.json

+{
+  "name": "jest-mock-scheduler",
+  "version": "0.1.0",
+  "description": "Jest matchers and utilities for testing the scheduler package.",


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

acdlite · 2018-11-30T23:17:49Z

packages/scheduler/src/Scheduler.js

@@ -447,41 +447,44 @@ var requestHostCallback;
 var cancelHostCallback;
 var shouldYieldToHost;

-if (typeof window !== 'undefined' && window._schedMock) {
+var globalValue = null;


Don't you mean globalThis? 😏

acdlite · 2018-11-30T23:21:28Z

scripts/rollup/bundles.js

+    moduleType: ISOMORPHIC,
+    entry: 'jest-mock-scheduler',
+    global: 'JestMockScheduler',
+    externals: ['jest-diff'],


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).

Probably copypasta too.

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

acdlite · 2018-11-30T23:22:50Z

packages/jest-mock-scheduler/index.js

+ * LICENSE file in the root directory of this source tree.
+ */
+
+'use strict';


Not related to this PR, but I don't think we need to put 'use strict' at the top of these files because they are ES modules, so strict mode is implied. @gaearon?

Cool. That was definitely just copypasta.

acdlite · 2018-11-30T23:23:30Z

packages/jest-mock-scheduler/README.md

@@ -0,0 +1,3 @@
+# `jest-mock-scheduler`
+
+Jest matchers and utilities for testing the `scheduler` package.


Probably should add note that this is experimental/unstable like we do with the others

ghost · 2018-12-20T18:45:42Z

Have you read the NPM terms?

A few examples of unacceptable content:
5. Content that exists only to "reserve" a name, whether a package name, user name, or organization name. The Dispute Policy governs how npm handles such cases of "squatting".

* Fixed scheduler setTimeout fallback * Moved unit-test-specific setTimeout code into a new NPM package, jest-mock-scheduler.

## Summary Removing package jest-mock-scheduler introduced in PR #14358, as it is no longer referenced in the main branch code. The following files previously referenced it: - packages/scheduler/src/__tests__/Scheduler-test.js - packages/scheduler/src/__tests__/SchedulerDOM-test.js - packages/shared/__tests__/ReactDOMFrameScheduling-test.js - scripts/jest/setupTests.js - scripts/rollup/bundles.js ## How did you test this change? ci green

## Summary Removing package jest-mock-scheduler introduced in PR #14358, as it is no longer referenced in the main branch code. The following files previously referenced it: - packages/scheduler/src/__tests__/Scheduler-test.js - packages/scheduler/src/__tests__/SchedulerDOM-test.js - packages/shared/__tests__/ReactDOMFrameScheduling-test.js - scripts/jest/setupTests.js - scripts/rollup/bundles.js ## How did you test this change? ci green DiffTrain build for [d7bb524](d7bb524) [View git log for this commit](https://github.com/facebook/react/commits/d7bb524ade26513ad310fedab5cad659012efd45)

## Summary Removing package jest-mock-scheduler introduced in PR facebook/react#14358, as it is no longer referenced in the main branch code. The following files previously referenced it: - packages/scheduler/src/__tests__/Scheduler-test.js - packages/scheduler/src/__tests__/SchedulerDOM-test.js - packages/shared/__tests__/ReactDOMFrameScheduling-test.js - scripts/jest/setupTests.js - scripts/rollup/bundles.js ## How did you test this change? ci green DiffTrain build for [d7bb524ade26513ad310fedab5cad659012efd45](facebook/react@d7bb524) [View git log for this commit](https://github.com/facebook/react/commits/d7bb524ade26513ad310fedab5cad659012efd45)

Fixed scheduler setTimeout fallback

b99e3e0

Moved unit-test-specific setTimeout code into a new NPM package, jest-mock-scheduler.

bvaughn requested a review from acdlite November 29, 2018 23:57

facebook-github-bot added the CLA Signed label Nov 29, 2018

bvaughn commented Nov 29, 2018

View reviewed changes

gaearon reviewed Nov 30, 2018

View reviewed changes

Combined window and global check for _schedMock

db14ad8

bvaughn force-pushed the scheduler-setTimeout-fix branch from 9a19b87 to db14ad8 Compare November 30, 2018 22:16

acdlite approved these changes Nov 30, 2018

View reviewed changes

Made Jest scheduler package private

2524836

bvaughn merged commit 52bea95 into facebook:master Dec 1, 2018

bvaughn deleted the scheduler-setTimeout-fix branch December 1, 2018 21:03

This was referenced Dec 1, 2018

🎣 Using React Hooks in React Native facebook/react-native#21967

Closed

Removed unnecessary externals from Jest bundles #14372

Merged

jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019

Fixed scheduler setTimeout fallback (facebook#14358)

a97834e

* Fixed scheduler setTimeout fallback * Moved unit-test-specific setTimeout code into a new NPM package, jest-mock-scheduler.

n8schloss pushed a commit to n8schloss/react that referenced this pull request Jan 31, 2019

Fixed scheduler setTimeout fallback (facebook#14358)

a5637cd

* Fixed scheduler setTimeout fallback * Moved unit-test-specific setTimeout code into a new NPM package, jest-mock-scheduler.

ymqy mentioned this pull request Jan 31, 2023

[cleanup] Remove unused package jest-mock-scheduler #26084

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed scheduler setTimeout fallback #14358

Fixed scheduler setTimeout fallback #14358

bvaughn commented Nov 29, 2018 •

edited

bvaughn Nov 29, 2018

gaearon Nov 30, 2018

bvaughn Nov 30, 2018 •

edited

bvaughn Nov 29, 2018

sizebot commented Nov 30, 2018 •

edited

jest-mock-scheduler

gaearon commented Nov 30, 2018

bvaughn commented Nov 30, 2018

gaearon commented Nov 30, 2018

bvaughn commented Nov 30, 2018

gaearon commented Nov 30, 2018 •

edited

bvaughn commented Nov 30, 2018

gaearon Nov 30, 2018

bvaughn Nov 30, 2018

gaearon Nov 30, 2018

bvaughn Nov 30, 2018 •

edited

acdlite Nov 30, 2018

bvaughn Dec 1, 2018

acdlite Nov 30, 2018

acdlite Nov 30, 2018

bvaughn Dec 1, 2018 •

edited

gaearon Dec 1, 2018

bvaughn Dec 2, 2018

bvaughn Dec 2, 2018

acdlite Nov 30, 2018

gaearon Nov 30, 2018

bvaughn Dec 1, 2018

acdlite Nov 30, 2018

ghost commented Dec 20, 2018 •

edited by ghost

		@@ -0,0 +1,3 @@
		# `jest-mock-scheduler`

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

Fixed scheduler setTimeout fallback #14358

Fixed scheduler setTimeout fallback #14358

Conversation

bvaughn commented Nov 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Nov 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sizebot commented Nov 30, 2018 • edited

jest-mock-scheduler

gaearon commented Nov 30, 2018

bvaughn commented Nov 30, 2018

gaearon commented Nov 30, 2018

bvaughn commented Nov 30, 2018

gaearon commented Nov 30, 2018 • edited

bvaughn commented Nov 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Nov 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Dec 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 20, 2018 • edited by ghost

bvaughn commented Nov 29, 2018 •

edited

bvaughn Nov 30, 2018 •

edited

sizebot commented Nov 30, 2018 •

edited

gaearon commented Nov 30, 2018 •

edited

bvaughn Nov 30, 2018 •

edited

bvaughn Dec 1, 2018 •

edited

ghost commented Dec 20, 2018 •

edited by ghost