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.

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

Remove buggy unstable_deferredUpdates() #13488

Merged
merged 4 commits into from
Aug 28, 2018
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 26, 2018

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

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

// 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 gaearon changed the title Add a failing test for deferredUpdates not being respected Don't consider deferredUpdates() inside interactive handler interactive Aug 27, 2018
@gaearon
Copy link
Collaborator Author

gaearon commented Aug 27, 2018

Pushed up a fix.

@NE-SmallTown
Copy link
Contributor

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
Copy link
Collaborator Author

gaearon commented Aug 27, 2018

I don't understand what you're asking.

@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Aug 27, 2018

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
Copy link
Collaborator Author

gaearon commented Aug 27, 2018

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
Copy link
Collaborator

acdlite commented Aug 27, 2018

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
Copy link
Collaborator Author

gaearon commented Aug 27, 2018

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
Copy link
Collaborator

acdlite commented Aug 27, 2018

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
Copy link

pull-bot commented Aug 27, 2018

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 gaearon changed the title Don't consider deferredUpdates() inside interactive handler interactive Remove buggy unstable_deferredUpdates() Aug 27, 2018
@@ -1568,11 +1568,14 @@ function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {
function deferredUpdates<A>(fn: () => A): A {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@gaearon gaearon merged commit 2967ebd into facebook:master Aug 28, 2018
@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Aug 29, 2018

@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
Copy link
Collaborator Author

gaearon commented Aug 29, 2018

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!

gaearon added a commit that referenced this pull request Aug 29, 2018
Addresses one of the issues brought up by @NE-SmallTown in #13488 (comment)
expect(syncValueRef.current.textContent).toBe('');

setUntrackedInputValue.call(inputRef.current, 'hello');
inputRef.current.dispatchEvent(new MouseEvent('input', {bubbles: true}));
Copy link
Contributor

Choose a reason for hiding this comment

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

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 👐

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks for the quick answer :)~

@gaearon gaearon mentioned this pull request Sep 5, 2018
facebook-github-bot pushed a commit to facebookarchive/nuclide that referenced this pull request Sep 13, 2018
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
pelmers added a commit to facebookarchive/atom-ide-ui that referenced this pull request Nov 15, 2018
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
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
* 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
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
Addresses one of the issues brought up by @NE-SmallTown in facebook#13488 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants