Remove buggy unstable_deferredUpdates() #13488

gaearon · 2018-08-26T16:48:34Z

This fixes the issue I was seeing in my JSConf demo, and that currently exists in the fixture:

react/fixtures/unstable-async/time-slicing/src/index.js

Lines 67 to 68 in 53ddcec

    
           // TODO: needing setTimeout here seems like a React bug. 
        
           setTimeout(() => {

react/fixtures/unstable-async/time-slicing/src/index.js

Lines 110 to 111 in 53ddcec

    
           // TODO: needing setTimeout here seems like a React bug. 
        
           setTimeout(() => {

~~When we enter deferredUpdates, we should treat them as deferred — regardless of whether we're in an interactive handler.~~

As per discussion below, we decided unstable_deferredUpdates is unnecessary since any update outside of event handlers in async mode is already treated as low priority. In userland code, the suggested solution for explicitly scheduling them will be scheduleWork (from the unreleased scheduler package). Until that package is published, requestIdleCallback or requestAnimationFrame work as alternatives.

gaearon · 2018-08-27T14:20:23Z

Pushed up a fix.

NE-SmallTown · 2018-08-27T14:33:57Z

Why deferred update depends on the InteractiveUpdates, it's like a trick? IMO, they are irrelevant(at least in the name), if we do this, maybe we need add some comments to the top of the SimpleEventPlugin.js

gaearon · 2018-08-27T14:54:45Z

I don't understand what you're asking.

NE-SmallTown · 2018-08-27T15:34:17Z

Sorry for my confusing! I mean, I don't understand why deferredUpdates() is relevant with the interactiveUpdate(Because this pr add isBatchingInteractiveUpdates to the deferredUpdates() function to distinguish the deferredUpdates(i.e. normal async update) and the interactive update, it makes deferredUpdates() relevant with the interactiveUpdate),

But interactive update is about event type like this and deferredUpdates() is not. So I'm a little confused because I don't see anything like event type in the naming or usage of deferredUpdates(). So I think if we do this, we need add some comments/tips to the SimpleEventPlugin(or other place) to explain this.

gaearon · 2018-08-27T15:42:55Z

Normally when we're inside an interactive handler (like onChange) we want setState in it to be treated as having interactive expiration.

However once we're inside deferredUpdates, it shouldn't be treated as interactive — regardless of whether we're currently handling an event or not. The only thing my fix is doing is to ensure that once we enter deferredUpdates, we "forget" that we were in an event handler, because the update is deferred anyway. Then we restore that information when we exit deferredUpdates.

acdlite · 2018-08-27T17:22:25Z

Can we just remove this API? We haven't found a legit use case for it that isn't solved by calling requestIdleCallback / scheduleWork.

gaearon · 2018-08-27T18:24:49Z

Is setState outside handlers already automatically lo pri in async mode? I didn’t realize this. If so, does it only happen if the component you set state on is inside an async tree? Or how else do we determine it should be lo pri by default (when today it’s sync by default)?

acdlite · 2018-08-27T20:50:51Z

Is setState outside handlers already automatically lo pri in async mode? I didn’t realize this. If so, does it only happen if the component you set state on is inside an async tree?

Yep that's how it works

pull-bot · 2018-08-27T20:57:06Z

ReactDOM: size: -0.1%, gzip: -0.1%

Details of bundled changes.

Comparing: 53ddcec...803ee47

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	-0.0%	-0.0%	649.5 KB	649.18 KB	152.6 KB	152.57 KB	UMD_DEV
react-dom.production.min.js	-0.1%	-0.1%	95.38 KB	95.27 KB	31.18 KB	31.15 KB	UMD_PROD
react-dom.development.js	-0.0%	-0.0%	645.64 KB	645.32 KB	151.49 KB	151.46 KB	NODE_DEV
react-dom.production.min.js	-0.1%	-0.1%	95.37 KB	95.26 KB	30.91 KB	30.89 KB	NODE_PROD
ReactDOM-dev.js	-0.0%	-0.0%	652.67 KB	652.35 KB	149.5 KB	149.46 KB	FB_WWW_DEV
ReactDOM-prod.js	-0.1%	-0.1%	288.17 KB	287.85 KB	53.32 KB	53.27 KB	FB_WWW_PROD
react-dom.profiling.min.js	-0.1%	-0.1%	96.56 KB	96.45 KB	31.31 KB	31.28 KB	NODE_PROFILING
ReactDOM-profiling.js	-0.1%	-0.1%	290.52 KB	290.2 KB	53.93 KB	53.89 KB	FB_WWW_PROFILING

Generated by 🚫 dangerJS

gaearon · 2018-08-27T21:01:13Z

packages/react-reconciler/src/ReactFiberScheduler.js

@@ -1568,11 +1568,14 @@ function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {
 function deferredUpdates<A>(fn: () => A): A {


Do you want to remove it from the scheduler/noop too?

NE-SmallTown · 2018-08-29T02:48:28Z

@gaearon Hi, dan, I have 2 questions here

SetState outside handlers already automatically lo pri in async mode. It only happen if the component you set state on is inside an async tree.

1.If so, all ways which could make us jump out the handlers like setTimeout, requestAnimationFrame and requestIdleCallback will works as expected. And I test(replace requestIdleCallback to setTimeout and requestAnimationFrame in the test case), it is.

So why we only choose "requestIdleCallback or requestAnimationFrame work as alternatives" (i.e. ignore the setTimeout way), and why you use requestIdleCallback rather than requestAnimationFrame in the test case.(Although from setupEnvironment.js they are the same because we don't use the timeRemaining param here).

2.Another question is, I'm a little confused with the "It only happen if the component you set state on is inside an async tree." But I don't see you use the <AsyncMode> in the test case of this pr, instead you use requestIdleCallback.

And if I add the <AsyncMode>(i.e. ReactDOM.render(<AsyncMode><Counter /></AsyncMode>, container);) to the test case of this pr, the first expect(syncValueRef.current.textContent).toBe('hello') is still true, that's very puzzling, IMO it should be false(i.e. syncValueRef.current.textContent should be '' because we are in the async mode just like others test case shows).

Wish you could clarify these 2 questions, Really appreciated, thanks!

gaearon · 2018-08-29T12:19:33Z

So why we only choose "requestIdleCallback or requestAnimationFrame work as alternatives" (i.e. ignore the setTimeout way), and why you use requestIdleCallback rather than requestAnimationFrame in the test case.(Although from setupEnvironment.js they are the same because we don't use the timeRemaining param here).

In practice we'll suggest people to use scheduleWork from our scheduler module instead. So this is temporary. You're right setTimeout works too.

2.Another question is, I'm a little confused with the "It only happen if the component you set state on is inside an async tree." But I don't see you use the <AsyncMode> in the test case of this pr, instead you use requestIdleCallback.

You're right. Initially the test was written for deferredUpdates in sync mode. But I just realized we can't make deferred updates in sync mode anymore at all. Which I guess is fine. But we need to tweak the test. I'll do that.

if I add the (i.e. ReactDOM.render(<AsyncMode><Counter /></AsyncMode>, container);) to the test case of this pr, the first expect(syncValueRef.current.textContent).toBe('hello') is still true, that's very puzzling

The update inside an "interactive" event, and it seems like we currently flush all interactive updates if an input was rendered. It's necessary for controlled inputs, but I'm not sure it makes sense for uncontrolled ones. I'll follow up.

Thank you!

@NE-SmallTown

Addresses one of the issues brought up by @NE-SmallTown in #13488 (comment)

Yurickh · 2018-08-29T12:33:13Z

packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.internal.js

+    expect(syncValueRef.current.textContent).toBe('');
+
+    setUntrackedInputValue.call(inputRef.current, 'hello');
+    inputRef.current.dispatchEvent(new MouseEvent('input', {bubbles: true}));


This might look like nitpicking, since it's completely unrelated to the issue at hand, but I'm honestly curious on why you used MouseEvent for input here.

Sorry for the random chime in 👐

I think I used click at first and then forgot to change when I rewrote the test

I see. Thanks for the quick answer :)~

Summary: The async mode unstable API deferredUpdates was removed in React 16.5.0 with facebook/react#13488 As discussed there, the alternative workaround is to simply use requestIdleCallback or requestAnimationFrame. Once a better API is released (tracking facebook/react#13306) we can use that instead for deferred async updates. Reviewed By: hansonw Differential Revision: D9814299 fbshipit-source-id: 8848cf05cd11ffba65b6d87233e2f5205121179b

* Add a failing test for deferredUpdates not being respected * Don't consider deferred updates interactive * Remove now-unnecessary setTimeout hack in the fixture * Remove unstable_deferredUpdates

@NE-SmallTown

Addresses one of the issues brought up by @NE-SmallTown in facebook#13488 (comment)

gaearon requested a review from acdlite August 26, 2018 16:48

facebook-github-bot added the CLA Signed label Aug 26, 2018

gaearon mentioned this pull request Aug 26, 2018

[Umbrella] Releasing Time Slicing #13306

Closed

13 tasks

mihalskiy approved these changes Aug 26, 2018

View reviewed changes

gaearon force-pushed the failing-async branch from 203e6ed to e32d23d Compare August 27, 2018 13:39

gaearon added 2 commits August 27, 2018 15:12

Add a failing test for deferredUpdates not being respected

f9c95ef

Don't consider deferred updates interactive

0a14e3b

gaearon force-pushed the failing-async branch from e32d23d to 0a14e3b Compare August 27, 2018 14:13

gaearon changed the title ~~Add a failing test for deferredUpdates not being respected~~ Don't consider deferredUpdates() inside interactive handler interactive Aug 27, 2018

Remove now-unnecessary setTimeout hack in the fixture

ebdca9f

acdlite approved these changes Aug 27, 2018

View reviewed changes

Remove unstable_deferredUpdates

803ee47

gaearon force-pushed the failing-async branch from b67ab91 to 803ee47 Compare August 27, 2018 20:58

gaearon changed the title ~~Don't consider deferredUpdates() inside interactive handler interactive~~ Remove buggy unstable_deferredUpdates() Aug 27, 2018

gaearon commented Aug 27, 2018

View reviewed changes

gaearon merged commit 2967ebd into facebook:master Aug 28, 2018

gaearon added a commit that referenced this pull request Aug 29, 2018

Fix test to use AsyncMode

1c0ba70

Addresses one of the issues brought up by @NE-SmallTown in #13488 (comment)

Yurickh reviewed Aug 29, 2018

View reviewed changes

gaearon mentioned this pull request Aug 29, 2018

Don't flush interactive updates to uncontrolled components early #13507

Closed

gaearon mentioned this pull request Sep 5, 2018

Changelog for 16.5.0 #13571

Merged

stipsan mentioned this pull request Sep 16, 2018

unstable_deferredUpdates is removed from react, replace with rAF reach/router#159

Merged

ashnur mentioned this pull request Sep 21, 2018

unstable_deferredUpdates was removed from react-dom koba04/react-fiber-resources#22

Closed

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

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

Fix test to use AsyncMode

ffda15d

Addresses one of the issues brought up by @NE-SmallTown in facebook#13488 (comment)

mpelekh mentioned this pull request Feb 24, 2019

[Fixtures] The removed ReactDOM.unstable_deferredUpdates method should be replaced in the fiber-triangle fixture #14945

Closed

Topthinking mentioned this pull request Apr 10, 2019

fixtures/fiber-triangle/index.html 144 line "unstable_deferredUpdates" undefined #15377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove buggy unstable_deferredUpdates() #13488

Remove buggy unstable_deferredUpdates() #13488

gaearon commented Aug 26, 2018 •

edited

gaearon commented Aug 27, 2018

NE-SmallTown commented Aug 27, 2018

gaearon commented Aug 27, 2018

NE-SmallTown commented Aug 27, 2018 •

edited

gaearon commented Aug 27, 2018

acdlite commented Aug 27, 2018

gaearon commented Aug 27, 2018

acdlite commented Aug 27, 2018

pull-bot commented Aug 27, 2018 •

edited

react-dom

gaearon Aug 27, 2018

NE-SmallTown commented Aug 29, 2018 •

edited

gaearon commented Aug 29, 2018

Yurickh Aug 29, 2018

gaearon Aug 29, 2018

Yurickh Aug 29, 2018

	// TODO: needing setTimeout here seems like a React bug.
	setTimeout(() => {

		@@ -1568,11 +1568,14 @@ function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {
		function deferredUpdates<A>(fn: () => A): A {

Remove buggy unstable_deferredUpdates() #13488

Remove buggy unstable_deferredUpdates() #13488

Conversation

gaearon commented Aug 26, 2018 • edited

gaearon commented Aug 27, 2018

NE-SmallTown commented Aug 27, 2018

gaearon commented Aug 27, 2018

NE-SmallTown commented Aug 27, 2018 • edited

gaearon commented Aug 27, 2018

acdlite commented Aug 27, 2018

gaearon commented Aug 27, 2018

acdlite commented Aug 27, 2018

pull-bot commented Aug 27, 2018 • edited

react-dom

gaearon Aug 27, 2018

Choose a reason for hiding this comment

NE-SmallTown commented Aug 29, 2018 • edited

gaearon commented Aug 29, 2018

Yurickh Aug 29, 2018

Choose a reason for hiding this comment

gaearon Aug 29, 2018

Choose a reason for hiding this comment

Yurickh Aug 29, 2018

Choose a reason for hiding this comment

gaearon commented Aug 26, 2018 •

edited

NE-SmallTown commented Aug 27, 2018 •

edited

pull-bot commented Aug 27, 2018 •

edited

NE-SmallTown commented Aug 29, 2018 •

edited