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.

Sign up for GitHub

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] 4/n Allow splitting out schedule in fb-www, prepare to fix polyfill issue internally #12900

Merged
merged 6 commits into from
May 29, 2018
Merged

[scheduler] 4/n Allow splitting out schedule in fb-www, prepare to fix polyfill issue internally #12900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7686bfc
Use local references to global things inside 'scheduler'
flarnie May 23, 2018
a96de36
Shim ReactScheduler for www
flarnie May 23, 2018
01c7385
Generate a bundle for fb-www
flarnie May 24, 2018
adef206
ran prettier
flarnie May 26, 2018
9c7b64d
fix rebase issues
flarnie May 26, 2018
1320890
Change names of variables used for holding globals
flarnie May 29, 2018
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
2 changes: 1 addition & 1 deletion packages/react-art/src/ReactARTHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import * as ReactScheduler from 'react-scheduler';
import * as ReactScheduler from 'shared/ReactScheduler';
import Transform from 'art/core/transform';
import Mode from 'art/modes/current';
import invariant from 'fbjs/lib/invariant';
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/client/ReactDOMHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @flow
*/

import * as ReactScheduler from 'react-scheduler';
import * as ReactScheduler from 'shared/ReactScheduler';

import * as ReactDOMComponentTree from './ReactDOMComponentTree';
import * as ReactDOMFiberComponent from './ReactDOMFiberComponent';
Expand Down
12 changes: 10 additions & 2 deletions packages/react-scheduler/src/ReactScheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,21 @@ export type CallbackIdType = CallbackConfigType;
import requestAnimationFrameForReact from 'shared/requestAnimationFrameForReact';
import ExecutionEnvironment from 'fbjs/lib/ExecutionEnvironment';

// We capture a local reference to any global, in case it gets polyfilled after
// this module is initially evaluated.
// We want to be using a consistent implementation.
const Date = global.Date;
const setTimeout = global.setTimeout;
const clearTimeout = global.clearTimeout;

const hasNativePerformanceNow =
typeof performance === 'object' && typeof performance.now === 'function';

let now;
if (hasNativePerformanceNow) {
const Performance = performance;
now = function() {
return performance.now();
return Performance.now();
};
} else {
now = function() {
Expand Down Expand Up @@ -224,7 +232,7 @@ if (!ExecutionEnvironment.canUseDOM) {
};
// Assumes that we have addEventListener in this environment. Might need
// something better for old IE.
window.addEventListener('message', idleTick, false);
global.addEventListener('message', idleTick, false);

const animationTick = function(rafTime) {
isAnimationFrameScheduled = false;
Expand Down
17 changes: 17 additions & 0 deletions packages/shared/ReactScheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

'use strict';
import {
now,
scheduleWork,
cancelScheduledWork,
} from 'react-scheduler/src/ReactScheduler';
Copy link
Collaborator

Choose a reason for hiding this comment

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

This indirection seems a bit complicated :-(

Do we want to keep this fork long term? Is there a better fix in mind? How will this ultimately work in open source when the package is ready?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is complicated, agreed.

Do we want to keep this fork long term? Is there a better fix in mind? How will this ultimately work in open source when the package is ready?

Here is my understanding of how it will work -

  • Once the schedule API is stable, we publish it as a separate module and make it a dependency of react-dom and react-art. In OS people just pull in the schedule bundle via npm, and it uses the native requestAnimationFrame.
  • We would still need something like this for the 'www' build of 'react-dom', because we still have the problem with our 'requestAnimationFrame' polyfill in 'www'. Some of the behavior of the polyfill doesn't work, but some of the custom behavior we still need. I think at that point we can get away with the following simpler approach in 'www':
    • we still require 'schedule' before the polyfill runs, which means 'schedule' pulls in the native rAF and uses it.
    • In 'www' we write a wrapper module that adds the 'timeSlice' wrapper to 'schedule' itself, which we still need, and React would grab the wrapped version. Not sure if we'll still need a fork/shim for that to work.

It took a lot of discussion to agree on this solution, I'm hesitant to go back to the drawing board when we have something that works.

Regarding my original solution of just shimming 'requestAnimationFrame' itself in our 'www' build - @sebmarkbage had concerns that we should not be using the polyfilled version of 'requestAnimationFrame' at all in 'www', and that the best way to avoid pulling in the polyfill was to require the schedule module before the polyfilling happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, if we just wrap the 'callback' before we pass it into 'scheduleWork' at every single callsite, or even if we just expose a wrapped version of 'schedule' for non-React use within 'www', then I think we can still avoid a fork of 'schedule'. After we publish it as a separate module, that is.


export {now, scheduleWork, cancelScheduledWork};
11 changes: 11 additions & 0 deletions packages/shared/forks/ReactScheduler.www.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

'use strict';
const {now, scheduleWork, cancelScheduledWork} = require('customSchedule');

export {now, scheduleWork, cancelScheduledWork};
5 changes: 5 additions & 0 deletions packages/shared/requestAnimationFrameForReact.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@
import ExecutionEnvironment from 'fbjs/lib/ExecutionEnvironment';
import warning from 'fbjs/lib/warning';

// We capture a local reference to any global, in case it gets polyfilled after
// this module is initially evaluated.
// We want to be using a consistent implementation.
const requestAnimationFrame = global.requestAnimationFrame;

if (__DEV__) {
if (
ExecutionEnvironment.canUseDOM &&
Expand Down
9 changes: 8 additions & 1 deletion scripts/rollup/bundles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,14 @@ const bundles = [
/******* React Scheduler (experimental) *******/
{
label: 'react-scheduler',
bundleTypes: [NODE_DEV, NODE_PROD, UMD_DEV, UMD_PROD],
bundleTypes: [
UMD_DEV,
UMD_PROD,
NODE_DEV,
NODE_PROD,
FB_WWW_DEV,
FB_WWW_PROD,
],
moduleType: ISOMORPHIC,
entry: 'react-scheduler',
global: 'ReactScheduler',
Expand Down
11 changes: 11 additions & 0 deletions scripts/rollup/forks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ const forks = Object.freeze({
},

// This logic is forked on www to use the 'acrossTransitions' version.
// This will be removed soon, see internal task T29442940
'shared/requestAnimationFrameForReact': (bundleType, entry) => {
switch (bundleType) {
case FB_WWW_DEV:
Expand All @@ -89,6 +90,16 @@ const forks = Object.freeze({
}
},

'shared/ReactScheduler': (bundleType, entry) => {
switch (bundleType) {
case FB_WWW_DEV:
case FB_WWW_PROD:
return 'shared/forks/ReactScheduler.www.js';
default:
return null;
}
},

// This logic is forked on www to blacklist warnings.
'shared/lowPriorityWarning': (bundleType, entry) => {
switch (bundleType) {
Expand Down