Skip to content

Refactor some controlled component stuff #26573

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

Merged
sebmarkbage merged 14 commits into from
Apr 9, 2023
Merged

Refactor some controlled component stuff #26573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
087f5e9
Rename controlled component methods
sebmarkbage Apr 7, 2023
f57beb1
Use nullish checks instead of hasOwnProperty which is generally our s…
sebmarkbage Apr 7, 2023
0e0af20
Move init and postInit together to show that this doesn't matter
sebmarkbage Apr 7, 2023
0c9c688
Move checkControlledValueProps to ReactDOMComponent
sebmarkbage Apr 7, 2023
e6f26c3
Fix tests
sebmarkbage Apr 8, 2023
a0a47e5
setDefaultValue is always guarded by a null check
sebmarkbage Apr 9, 2023
1ad8392
Since we just initialized the initial state, we can infer it from the…
sebmarkbage Apr 9, 2023
17abdbe
Move controlled warning to be done by last/next props
sebmarkbage Apr 9, 2023
b3e97c5
Delete wrapperState
sebmarkbage Apr 9, 2023
63b40e5
Textarea only initializes the wrapper state to read it at the same time
sebmarkbage Apr 9, 2023
18a5a9f
Remove wrapperState from select
sebmarkbage Apr 9, 2023
2019ddc
Delete value attribute when defaultValue sets back to null/missing
sebmarkbage Apr 9, 2023
b263fa3
Reset textarea to empty after defaultValue is removed or null
sebmarkbage Apr 9, 2023
91f7014
Rename initX to validateXProps and postInitX to initX
sebmarkbage Apr 9, 2023
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
148 changes: 102 additions & 46 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
* @flow
*/

import type {InputWithWrapperState} from './ReactDOMInput';

import {
registrationNameDependencies,
possibleRegistrationNames,
Expand All @@ -17,6 +15,7 @@ import {
import {canUseDOM} from 'shared/ExecutionEnvironment';
import {checkHtmlStringCoercion} from 'shared/CheckStringCoercion';
import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion';
import {checkControlledValueProps} from '../shared/ReactControlledValuePropTypes';

import {
getValueForAttribute,
Expand All @@ -27,27 +26,24 @@ import {
setValueForNamespacedAttribute,
} from './DOMPropertyOperations';
import {
initWrapperState as ReactDOMInputInitWrapperState,
postMountWrapper as ReactDOMInputPostMountWrapper,
updateChecked as ReactDOMInputUpdateChecked,
updateWrapper as ReactDOMInputUpdateWrapper,
restoreControlledState as ReactDOMInputRestoreControlledState,
validateInputProps,
initInput,
updateInputChecked,
updateInput,
restoreControlledInputState,
} from './ReactDOMInput';
import {initOption, validateOptionProps} from './ReactDOMOption';
import {
postMountWrapper as ReactDOMOptionPostMountWrapper,
validateProps as ReactDOMOptionValidateProps,
} from './ReactDOMOption';
import {
initWrapperState as ReactDOMSelectInitWrapperState,
postMountWrapper as ReactDOMSelectPostMountWrapper,
restoreControlledState as ReactDOMSelectRestoreControlledState,
postUpdateWrapper as ReactDOMSelectPostUpdateWrapper,
validateSelectProps,
initSelect,
restoreControlledSelectState,
updateSelect,
} from './ReactDOMSelect';
import {
initWrapperState as ReactDOMTextareaInitWrapperState,
postMountWrapper as ReactDOMTextareaPostMountWrapper,
updateWrapper as ReactDOMTextareaUpdateWrapper,
restoreControlledState as ReactDOMTextareaRestoreControlledState,
validateTextareaProps,
initTextarea,
updateTextarea,
restoreControlledTextareaState,
} from './ReactDOMTextarea';
import {track} from './inputValueTracking';
import setInnerHTML from './setInnerHTML';
Expand Down Expand Up @@ -79,6 +75,8 @@ import {
listenToNonDelegatedEvent,
} from '../events/DOMPluginEventSystem';

let didWarnControlledToUncontrolled = false;
let didWarnUncontrolledToControlled = false;
let didWarnInvalidHydration = false;
let canDiffStyleForHydrationWarning;
if (__DEV__) {
Expand Down Expand Up @@ -805,7 +803,9 @@ export function setInitialProperties(
break;
}
case 'input': {
ReactDOMInputInitWrapperState(domElement, props);
if (__DEV__) {
checkControlledValueProps('input', props);
}
// We listen to this event in case to ensure emulated bubble
// listeners still fire for the invalid event.
listenToNonDelegatedEvent('invalid', domElement);
Expand Down Expand Up @@ -834,10 +834,10 @@ export function setInitialProperties(
break;
}
case 'checked': {
const node = ((domElement: any): InputWithWrapperState);
const checked =
propValue != null ? propValue : node._wrapperState.initialChecked;
node.checked =
propValue != null ? propValue : props.defaultChecked;
const inputElement: HTMLInputElement = (domElement: any);
inputElement.checked =
!!checked &&
typeof checked !== 'function' &&
checked !== 'symbol';
Expand Down Expand Up @@ -866,11 +866,14 @@ export function setInitialProperties(
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
track((domElement: any));
ReactDOMInputPostMountWrapper(domElement, props, false);
validateInputProps(domElement, props);
initInput(domElement, props, false);
return;
}
case 'select': {
ReactDOMSelectInitWrapperState(domElement, props);
if (__DEV__) {
checkControlledValueProps('select', props);
}
// We listen to this event in case to ensure emulated bubble
// listeners still fire for the invalid event.
listenToNonDelegatedEvent('invalid', domElement);
Expand All @@ -893,11 +896,14 @@ export function setInitialProperties(
}
}
}
ReactDOMSelectPostMountWrapper(domElement, props);
validateSelectProps(domElement, props);
initSelect(domElement, props);
return;
}
case 'textarea': {
ReactDOMTextareaInitWrapperState(domElement, props);
if (__DEV__) {
checkControlledValueProps('textarea', props);
}
// We listen to this event in case to ensure emulated bubble
// listeners still fire for the invalid event.
listenToNonDelegatedEvent('invalid', domElement);
Expand Down Expand Up @@ -936,11 +942,12 @@ export function setInitialProperties(
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
track((domElement: any));
ReactDOMTextareaPostMountWrapper(domElement, props);
validateTextareaProps(domElement, props);
initTextarea(domElement, props);
return;
}
case 'option': {
ReactDOMOptionValidateProps(domElement, props);
validateOptionProps(domElement, props);
for (const propKey in props) {
if (!props.hasOwnProperty(propKey)) {
continue;
Expand All @@ -963,7 +970,7 @@ export function setInitialProperties(
}
}
}
ReactDOMOptionPostMountWrapper(domElement, props);
initOption(domElement, props);
return;
}
case 'dialog': {
Expand Down Expand Up @@ -1213,17 +1220,17 @@ export function updateProperties(
// In the middle of an update, it is possible to have multiple checked.
// When a checked radio tries to change name, browser makes another radio's checked false.
if (nextProps.type === 'radio' && nextProps.name != null) {
ReactDOMInputUpdateChecked(domElement, nextProps);
updateInputChecked(domElement, nextProps);
}
for (let i = 0; i < updatePayload.length; i += 2) {
const propKey = updatePayload[i];
const propValue = updatePayload[i + 1];
switch (propKey) {
case 'checked': {
const node = ((domElement: any): InputWithWrapperState);
const checked =
propValue != null ? propValue : node._wrapperState.initialChecked;
node.checked =
propValue != null ? propValue : nextProps.defaultChecked;
const inputElement: HTMLInputElement = (domElement: any);
inputElement.checked =
!!checked &&
typeof checked !== 'function' &&
checked !== 'symbol';
Expand All @@ -1249,10 +1256,50 @@ export function updateProperties(
}
}
}

if (__DEV__) {
const wasControlled =
lastProps.type === 'checkbox' || lastProps.type === 'radio'
? lastProps.checked != null
: lastProps.value != null;
const isControlled =
nextProps.type === 'checkbox' || nextProps.type === 'radio'
? nextProps.checked != null
: nextProps.value != null;

if (
!wasControlled &&
isControlled &&
!didWarnUncontrolledToControlled
) {
console.error(
'A component is changing an uncontrolled input to be controlled. ' +
'This is likely caused by the value changing from undefined to ' +
'a defined value, which should not happen. ' +
'Decide between using a controlled or uncontrolled input ' +
'element for the lifetime of the component. More info: https://reactjs.org/link/controlled-components',
);
didWarnUncontrolledToControlled = true;
}
if (
wasControlled &&
!isControlled &&
!didWarnControlledToUncontrolled
) {
console.error(
'A component is changing a controlled input to be uncontrolled. ' +
'This is likely caused by the value changing from a defined to ' +
'undefined, which should not happen. ' +
'Decide between using a controlled or uncontrolled input ' +
'element for the lifetime of the component. More info: https://reactjs.org/link/controlled-components',
);
didWarnControlledToUncontrolled = true;
}
}
// Update the wrapper around inputs *after* updating props. This has to
// happen after updating the rest of props. Otherwise HTML5 input validations
// raise warnings and prevent the new value from being assigned.
ReactDOMInputUpdateWrapper(domElement, nextProps);
updateInput(domElement, nextProps);
return;
}
case 'select': {
Expand All @@ -1272,7 +1319,7 @@ export function updateProperties(
}
// <select> value update needs to occur after <option> children
// reconciliation
ReactDOMSelectPostUpdateWrapper(domElement, nextProps);
updateSelect(domElement, lastProps, nextProps);
return;
}
case 'textarea': {
Expand Down Expand Up @@ -1303,7 +1350,7 @@ export function updateProperties(
}
}
}
ReactDOMTextareaUpdateWrapper(domElement, nextProps);
updateTextarea(domElement, nextProps);
return;
}
case 'option': {
Expand Down Expand Up @@ -2263,38 +2310,47 @@ export function diffHydratedProperties(
listenToNonDelegatedEvent('toggle', domElement);
break;
case 'input':
ReactDOMInputInitWrapperState(domElement, props);
if (__DEV__) {
checkControlledValueProps('input', props);
}
// We listen to this event in case to ensure emulated bubble
// listeners still fire for the invalid event.
listenToNonDelegatedEvent('invalid', domElement);
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
track((domElement: any));
validateInputProps(domElement, props);
// For input and textarea we current always set the value property at
// post mount to force it to diverge from attributes. However, for
// option and select we don't quite do the same thing and select
// is not resilient to the DOM state changing so we don't do that here.
// TODO: Consider not doing this for input and textarea.
Copy link
Contributor Author

@sebmarkbage sebmarkbage Apr 8, 2023

Choose a reason for hiding this comment

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

There's something off about these inconsistencies. We probably should be issuing change events if the value has changed before hydration and then reset using the "restore" mechanism. But the fact that select is different is also weird. Does that mean that controlled selects are not updated to reflect the "state" if it changes before hydration. Seems like it.

If we ironed this out we could decide whether the init should be merged with the postInit.

Copy link
Contributor Author

@sebmarkbage sebmarkbage Apr 8, 2023

Choose a reason for hiding this comment

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

I think maybe the other bug here is that we call postInit during hydration, and not in the commit phase in Fiber which seems wrong too. If it was in the commit phase the "mount" name would make more sense.

ReactDOMInputPostMountWrapper(domElement, props, true);
initInput(domElement, props, true);
break;
case 'option':
ReactDOMOptionValidateProps(domElement, props);
validateOptionProps(domElement, props);
break;
case 'select':
ReactDOMSelectInitWrapperState(domElement, props);
if (__DEV__) {
checkControlledValueProps('select', props);
}
// We listen to this event in case to ensure emulated bubble
// listeners still fire for the invalid event.
listenToNonDelegatedEvent('invalid', domElement);
validateSelectProps(domElement, props);
break;
case 'textarea':
ReactDOMTextareaInitWrapperState(domElement, props);
if (__DEV__) {
checkControlledValueProps('textarea', props);
}
// We listen to this event in case to ensure emulated bubble
// listeners still fire for the invalid event.
listenToNonDelegatedEvent('invalid', domElement);
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
track((domElement: any));
ReactDOMTextareaPostMountWrapper(domElement, props);
validateTextareaProps(domElement, props);
initTextarea(domElement, props);
break;
}

Expand Down Expand Up @@ -2472,13 +2528,13 @@ export function restoreControlledState(
): void {
switch (tag) {
case 'input':
ReactDOMInputRestoreControlledState(domElement, props);
restoreControlledInputState(domElement, props);
return;
case 'textarea':
ReactDOMTextareaRestoreControlledState(domElement, props);
restoreControlledTextareaState(domElement, props);
return;
case 'select':
ReactDOMSelectRestoreControlledState(domElement, props);
restoreControlledSelectState(domElement, props);
return;
}
}
Loading