[WIP] Add requestIdleCallback polyfill package #12253

aweary · 2018-02-20T18:11:27Z

Adds a new package, request-idle-callback-polyfill (final name pending) that's based on the polyfill inside of ReactDOMFrameScheduling. Adds support for scheduling multiple callbacks. It also schedules a setTimeout call to ensure that callbacks with timeouts get called.

TODO

create two entry points, one that writes to global and one that doesn't
See if we can detect native rIC implementations and warn when people do the wrong thing
figure out the actual name for the package
figure out the correct build config for rollup/bundles.js (probably don't need prod/dev builds?)
determine the final API ¹
remove the previous polyfill from ReactDOMFrameScheduling and import this one
- Depending on this answer, potentially re-export the native requestIdleCallback if it exists

1: For example, should it export requestIdleCallback or write to window?

Sets up the basic structure for the new package along with some empty tests. Pulls out the relevant code from ReactDOMFrameScheduling as a starting point for the polyfill.

This makes it a little easier understand when trying to implement spec-compliant behavior

This is a prerequisite of supporting multiple scheduled callbacks.

idleTick calculates whether a callback is timed out, but this still depends on the callback queue being processed. Use setTimeout as a fallback so that callbacks with timeouts definitely get called within a reasonable timeframe.

This was used when only a single callback was supported

acdlite · 2018-02-20T18:16:20Z

(probably don't need prod/dev builds?)

Why not?

pull-bot · 2018-02-20T18:19:43Z

Details of bundled changes.

Comparing: 48ffbf0...b3a5223

request-idle-callback-polyfill

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
request-idle-callback-polyfill.development.js	+100%	+100%	0 B	6.88 KB	0 B	2.5 KB	UMD_DEV
request-idle-callback-polyfill.production.min.js	🔺+100%	🔺+100%	0 B	1.59 KB	0 B	907 B	UMD_PROD
request-idle-callback-polyfill.development.js	+100%	+100%	0 B	6.68 KB	0 B	2.45 KB	NODE_DEV
request-idle-callback-polyfill.production.min.js	🔺+100%	🔺+100%	0 B	1.42 KB	0 B	839 B	NODE_PROD

Generated by 🚫 dangerJS

acdlite · 2018-02-20T18:20:43Z

packages/request-idle-callback-polyfill/npm/index.js

+'use strict';
+
+// @TODO figure out if we need a prod/dev build for this?
+// if (process.env.NODE_ENV === 'production') {


Yes :D I'm curious why you thought perhaps we don't need separate builds?

What would be the advantage of a development build? There aren't any warnings or DEV-only checks as of now. I guess just providing a non-minified build in DEV is a fine enough reason?

I see. Yeah, let's have both, for debugging. And since it's possible we may add warnings.

This may limit adoption and someone might publish one that just has the one. The prod/dev build thing is still somewhat controversial.

The whole point of this is that we would like this particular implementation to be canonical and gain adoption so a complex build works against that.

Why do we care about anyone who isn’t also using React, with which they have to deal with this anyway?

I get that you’re saying we can get more adoption within the React community if it becomes widely adopted outside of it but idk if I buy that scenario

In my view the main argument for a DEV build is so we can warn, e.g. if we detect the presence of a non-native global requestIdleCallback implementation, which isn’t something we’d want to do in prod (I’m assuming)

Polyfills are not always part of the same build system neither since they can often just be concatenated or loaded from a polyfill CDN service. Even if you're using React you'd hit this.

I just don't see a reason for it.

The scenario you're describing is just a warning about two polyfills existing which you shouldn't do anyway. Even if we warn, there might be another one after that overrides.

We can still warn for a non-native global requestIdleCallback implementation in React itself. That's where it matters.

Warning in ReactDOM makes sense.

acdlite · 2018-02-20T18:25:27Z

packages/request-idle-callback-polyfill/src/__tests__/RequestIdleCallback-test.js

+
+    it('executes callbacks that timeout', () => {
+      const callback = jest.fn(deadline => {
+        expect(deadline.didTimeout).toBe(true);


Our typical strategy is to never put assertions inside of callbacks. It's too easy to neglect to call the callback. Instead, we push into a log and make assertions on it in the main function. See one of our async tests for an example:

react/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js

Lines 75 to 140 in 48ffbf0

it('updates a previous render', () => {

let ops = [];

function Header() {

ops.push('Header');

return <h1>Hi</h1>;

}

function Content(props) {

ops.push('Content');

return <div>{props.children}</div>;

}

function Footer() {

ops.push('Footer');

return <footer>Bye</footer>;

}

const header = <Header />;

const footer = <Footer />;

function Foo(props) {

ops.push('Foo');

return (

<div>

{header}

<Content>{props.text}</Content>

{footer}

</div>

);

}

ReactNoop.render(<Foo text="foo" />, () =>

ops.push('renderCallbackCalled'),

);

ReactNoop.flush();

expect(ops).toEqual([

'Foo',

'Header',

'Content',

'Footer',

'renderCallbackCalled',

]);

ops = [];

ReactNoop.render(<Foo text="bar" />, () =>

ops.push('firstRenderCallbackCalled'),

);

ReactNoop.render(<Foo text="bar" />, () =>

ops.push('secondRenderCallbackCalled'),

);

ReactNoop.flush();

// TODO: Test bail out of host components. This is currently unobservable.

// Since this is an update, it should bail out and reuse the work from

// Header and Content.

expect(ops).toEqual([

'Foo',

'Content',

'firstRenderCallbackCalled',

'secondRenderCallbackCalled',

]);

});

Then you can get rid of suppressErrorLogging = false.

It does assert that the callback is called, but this looks much better than hacking suppressErrorLogging 👌

Another benefit of a log is the test will fail if there are multiple invocations.

TrySound · 2018-02-21T21:33:29Z

packages/request-idle-callback-polyfill/package.json

-      "README.md",
-      "index.js"
-    ],
+    "files": ["LICENSE", "README.md", "index.js", "cjs/"],


Is it ok to format package.json?

While it's reasonable for the polyfill to assume a DOM environment, ReactDOM itself must be able to run in a Node environment. These polyfills won't actually be called if it is, so making any DOM-specific initialization lazy protects us from breaking Node.

acdlite · 2018-02-23T20:57:27Z

I'm curious to see what your test plan will be

aweary · 2018-02-23T23:21:35Z

@acdlite I was going to start with the expiration fixture. Any other recommendations?

acdlite · 2018-02-24T01:22:55Z

Yeah some sort of fixture seems like the way to go

sebmarkbage

We might want to externalize but if we do, it'll be under a different name and not as a polyfill.

EECOLOR · 2018-03-26T20:42:38Z

Not sure if this is related. I have searched, but could not find a good place to comment. My hope is that this proposal actually answers my question.

I am working on a custom renderer and noticed ReactDOMFrameScheduling has some great methods when you are in a browser. My original question was to make it publicly available (probably as part of react-dom). It seems this thread makes the original request obsolete.

If my assumption is correct you can consider this a use case to support moving the logic to a separate package.

aweary · 2018-03-27T16:21:12Z

@sebmarkbage so what does that mean for this PR? I'm happy to restart the effort with a different approach if this isn't what we want to do.

aweary · 2018-04-17T00:50:58Z

Superseded by the much cooler sounding React Scheduler #12624

Brandon Dail added 9 commits February 20, 2018 09:59

Create request-idle-callback-polyfill package

11f6b72

Sets up the basic structure for the new package along with some empty tests. Pulls out the relevant code from ReactDOMFrameScheduling as a starting point for the polyfill.

Add some spec-defined types

09ec46e

Rename local variables to be consistent with spec

882ab49

This makes it a little easier understand when trying to implement spec-compliant behavior

Recreate frameDeadlineObj every time a callback is called

fe28f0a

This is a prerequisite of supporting multiple scheduled callbacks.

Add support for scheduling multiple callbacks

4a5134f

Use setTimeout to call timed out callbacks

f1be277

idleTick calculates whether a callback is timed out, but this still depends on the callback queue being processed. Use setTimeout as a fallback so that callbacks with timeouts definitely get called within a reasonable timeframe.

Implement test suite

7bae7f8

Run prettier

c6cb758

Remove scheduledRICCallback

1a0d25f

This was used when only a single callback was supported

facebook-github-bot added the CLA Signed label Feb 20, 2018

acdlite reviewed Feb 20, 2018

View reviewed changes

Brandon Dail added 3 commits February 21, 2018 10:58

Remove assertions from mock idle callback

18a57f4

Export a PROD and DEV build

299737d

Include cjs/ in package.json file

cd1bfc7

TrySound reviewed Feb 21, 2018

View reviewed changes

Brandon Dail added 2 commits February 21, 2018 14:28

Use request-idle-callback polyfill in ReactDOMFrameScheduling

b3a5223

sebmarkbage requested changes Mar 15, 2018

View reviewed changes

aweary mentioned this pull request Apr 16, 2018

React Scheduler - Initial commit #12624

Merged

aweary closed this Apr 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add requestIdleCallback polyfill package #12253

[WIP] Add requestIdleCallback polyfill package #12253

aweary commented Feb 20, 2018 •

edited

Loading

acdlite commented Feb 20, 2018

pull-bot commented Feb 20, 2018 •

edited

Loading

request-idle-callback-polyfill

acdlite Feb 20, 2018

aweary Feb 20, 2018

acdlite Feb 20, 2018

sebmarkbage Feb 23, 2018

acdlite Feb 23, 2018

acdlite Feb 23, 2018

acdlite Feb 23, 2018

sebmarkbage Feb 23, 2018

acdlite Feb 23, 2018

acdlite Feb 20, 2018

aweary Feb 20, 2018

acdlite Feb 20, 2018

TrySound Feb 21, 2018

acdlite commented Feb 23, 2018

aweary commented Feb 23, 2018

acdlite commented Feb 24, 2018

sebmarkbage left a comment

EECOLOR commented Mar 26, 2018

aweary commented Mar 27, 2018

aweary commented Apr 17, 2018

	it('updates a previous render', () => {
	let ops = [];

	function Header() {
	ops.push('Header');
	return <h1>Hi</h1>;
	}

	function Content(props) {
	ops.push('Content');
	return <div>{props.children}</div>;
	}

	function Footer() {
	ops.push('Footer');
	return <footer>Bye</footer>;
	}

	const header = <Header />;
	const footer = <Footer />;

	function Foo(props) {
	ops.push('Foo');
	return (
	<div>
	{header}
	<Content>{props.text}</Content>
	{footer}
	</div>
	);
	}

	ReactNoop.render(<Foo text="foo" />, () =>
	ops.push('renderCallbackCalled'),
	);
	ReactNoop.flush();

	expect(ops).toEqual([
	'Foo',
	'Header',
	'Content',
	'Footer',
	'renderCallbackCalled',
	]);

	ops = [];

	ReactNoop.render(<Foo text="bar" />, () =>
	ops.push('firstRenderCallbackCalled'),
	);
	ReactNoop.render(<Foo text="bar" />, () =>
	ops.push('secondRenderCallbackCalled'),
	);
	ReactNoop.flush();

	// TODO: Test bail out of host components. This is currently unobservable.

	// Since this is an update, it should bail out and reuse the work from
	// Header and Content.
	expect(ops).toEqual([
	'Foo',
	'Content',
	'firstRenderCallbackCalled',
	'secondRenderCallbackCalled',
	]);
	});

[WIP] Add requestIdleCallback polyfill package #12253

[WIP] Add requestIdleCallback polyfill package #12253

Conversation

aweary commented Feb 20, 2018 • edited Loading

TODO

acdlite commented Feb 20, 2018

pull-bot commented Feb 20, 2018 • edited Loading

request-idle-callback-polyfill

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

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

acdlite commented Feb 23, 2018

aweary commented Feb 23, 2018

acdlite commented Feb 24, 2018

sebmarkbage left a comment

Choose a reason for hiding this comment

EECOLOR commented Mar 26, 2018

aweary commented Mar 27, 2018

aweary commented Apr 17, 2018

aweary commented Feb 20, 2018 •

edited

Loading

pull-bot commented Feb 20, 2018 •

edited

Loading