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

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

Closed
wants to merge 2 commits into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 29, 2018

This is related to what @NE-SmallTown noticed in #13488 (comment).

In the first commit, I amend my test to check for more things. Specifically, it seems weird that setState inside an onChange handler in async mode was flushed synchronously upon exiting an event — even though there was just one event. So I assert that it shouldn’t do that.

The cause turns out related to this:

const controlledComponentsHavePendingUpdates = needsStateRestore();
if (controlledComponentsHavePendingUpdates) {
// If a controlled event was fired, we may need to restore the state of
// the DOM node back to the controlled value. This is necessary when React
// bails out of the update without touching the DOM.
_flushInteractiveUpdatesImpl();
restoreStateIfNeeded();
}

We need to flush restore for controlled inputs at the end of the interactive event. But we currently do this regardless of whether an input is controlled or not:

// Flag this event loop as needing state restore.
enqueueStateRestore(target);

In the second commit, I change it so that we don't flush interactive updates early for cases where the inputs aren't controlled (or affect other controlled inputs).

I'm not very familiar with this code and not entirely sure this is right. For example, an input might flip from uncontrolled to controlled, and we kinda support that (with a warning)? Then I think this detection would fail.

Also not sure it's worth the added complexity in practice. If you think it's valuable I can add more test coverage.

This test fails because we enqueue state restore for any input -- even uncontrolled.
@pull-bot
Copy link

pull-bot commented Aug 29, 2018

ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.2%

Details of bundled changes.

Comparing: 1c0ba70...b9b8a62

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 660.24 KB 660.85 KB 154.94 KB 155.1 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.2% 95.36 KB 95.46 KB 31.18 KB 31.25 KB UMD_PROD
react-dom.development.js +0.1% +0.1% 656.33 KB 656.94 KB 153.81 KB 153.97 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 95.3 KB 95.4 KB 30.91 KB 30.94 KB NODE_PROD
ReactDOM-dev.js +0.1% +0.1% 663.69 KB 664.32 KB 151.92 KB 152.08 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.1% 🔺+0.1% 287.74 KB 287.99 KB 53.32 KB 53.35 KB FB_WWW_PROD
react-dom.profiling.min.js +0.1% +0.1% 98.18 KB 98.28 KB 31.51 KB 31.54 KB NODE_PROFILING
ReactDOM-profiling.js +0.1% +0.1% 293.15 KB 293.39 KB 54.64 KB 54.68 KB FB_WWW_PROFILING

Generated by 🚫 dangerJS

@sebmarkbage
Copy link
Collaborator

Doesn’t seem worth it to me.

Uncontrolled Components are non-idiomatic and not something we optimize for. Certainly don’t want to add more code for.

Interactive updates do not provide a lot of guarantees of batching regardless. If you want that you use scheduleWork.

We know we want to make a breaking change around this anyway in #9657 so we can fix it then rather than risk it now.

@sebmarkbage
Copy link
Collaborator

We still have to explain how to use scheduleWork in controlled components to avoid this. So it’s just simpler that it’s the same story for both.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 29, 2018

Fair.

@gaearon gaearon closed this Aug 29, 2018
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.

4 participants