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

Conversation

Projects
None yet
6 participants
@bvaughn
Copy link
Contributor

bvaughn commented Nov 29, 2018

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.

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

@bvaughn bvaughn requested a review from acdlite Nov 29, 2018

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

This comment has been minimized.

@bvaughn

bvaughn Nov 29, 2018

Contributor

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

This comment has been minimized.

@gaearon

gaearon Nov 30, 2018

Member

Let's combine.

This comment has been minimized.

@bvaughn

bvaughn Nov 30, 2018

Contributor

Sure, combined in db14ad8

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

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

This comment has been minimized.

@bvaughn

bvaughn Nov 29, 2018

Contributor

This convention seemed fairly inline with existing Jest mock packages.

@sizebot

This comment has been minimized.

Copy link

sizebot commented Nov 30, 2018

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

This comment has been minimized.

Copy link
Member

gaearon commented Nov 30, 2018

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

This comment has been minimized.

Copy link
Contributor

bvaughn commented Nov 30, 2018

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

This comment has been minimized.

Copy link
Member

gaearon commented Nov 30, 2018

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

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Nov 30, 2018

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

This comment has been minimized.

Copy link
Member

gaearon commented Nov 30, 2018

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

This comment has been minimized.

Copy link
Contributor

bvaughn commented Nov 30, 2018

@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 😄

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

This comment has been minimized.

@gaearon

gaearon Nov 30, 2018

Member

What is this?

This comment has been minimized.

@bvaughn

bvaughn Nov 30, 2018

Contributor

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

This comment has been minimized.

@gaearon

gaearon Nov 30, 2018

Member

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

This comment has been minimized.

@bvaughn

bvaughn Nov 30, 2018

Contributor

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.

@bvaughn bvaughn force-pushed the bvaughn:scheduler-setTimeout-fix branch from 9a19b87 to db14ad8 Nov 30, 2018

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

This comment has been minimized.

@acdlite

acdlite Nov 30, 2018

Member

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

This comment has been minimized.

@bvaughn

bvaughn Dec 1, 2018

Contributor

Sure!

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

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

This comment has been minimized.

@acdlite

acdlite Nov 30, 2018

Member

Don't you mean globalThis? 😏

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

This comment has been minimized.

@acdlite

acdlite Nov 30, 2018

Member

Copypasta

This comment has been minimized.

@bvaughn

bvaughn Dec 1, 2018

Contributor

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

This comment has been minimized.

@gaearon

gaearon Dec 1, 2018

Member

Probably copypasta too.

This comment has been minimized.

@bvaughn

bvaughn Dec 2, 2018

Contributor

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

This comment has been minimized.

@bvaughn
* LICENSE file in the root directory of this source tree.
*/

'use strict';

This comment has been minimized.

@acdlite

acdlite Nov 30, 2018

Member

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?

This comment has been minimized.

@gaearon

This comment has been minimized.

@bvaughn

bvaughn Dec 1, 2018

Contributor

Cool. That was definitely just copypasta.

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

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

This comment has been minimized.

@acdlite

acdlite Nov 30, 2018

Member

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

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

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@bvaughn bvaughn deleted the bvaughn:scheduler-setTimeout-fix branch Dec 1, 2018

@def14nt

This comment has been minimized.

Copy link

def14nt commented Dec 20, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment