Use already extracted values instead of reading off props for controlled components #26596
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: ac43bf6...79e4988 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
sebmarkbage
force-pushed
the
singlelooppattern
branch
from
d12024b
to
79e4988
Compare
| let type = null; | ||
| let value = null; | ||
| let defaultValue = null; | ||
| let checked = null; | ||
| let defaultChecked = null; |
There was a problem hiding this comment.
you wrote these 5 variables in 3 different orders in the same function
There was a problem hiding this comment.
You mean in the argument order? The assignments are copy-paste.
| (nextProp != null || lastProp != null) | ||
| ) { | ||
| switch (propKey) { | ||
| case 'type': { | ||
| type = nextProp; | ||
| // Fast path since 'type' is very common on inputs |
There was a problem hiding this comment.
Changing 'type' is very uncommon though. setProp would be fewer bytes?
There was a problem hiding this comment.
I want to change this to domElement.type = nextProp but I leave that for a later refactor as well as dropping function and symbol as special cases which would then bring this down. For now I'm just highlighting that these special cases apply often. setProp would obscure that this should actually be simple.
| value: ?string, | ||
| defaultValue: ?string, |
There was a problem hiding this comment.
Yes I mainly did this to ensure that Flow errors if you supply them out of order, which it doesn't really do anyway since the input is any most of the time.
| value: ?string, | ||
| defaultValue: ?string, |
| if (isButton || value !== node.value) { | ||
| node.value = toString(value); | ||
| if (isButton || toString(getToStringValue(value)) !== node.value) { | ||
| node.value = toString(getToStringValue(value)); |
There was a problem hiding this comment.
is it wasteful to stringify these many times?
There was a problem hiding this comment.
Yea but we already were in a bunch of place. I'm looking to get rid of these helpers in a follow up and align it more with how attributes do it.
| @@ -203,8 +198,8 @@ export function initInput( | |||
| // prematurely marking required inputs as invalid. Equality is compared | |||
| // to the current value in case the browser provided value is not an | |||
| // empty string. | |||
| if (isButton || value !== node.value) { | |||
| node.value = toString(value); | |||
| if (isButton || toString(getToStringValue(value)) !== node.value) { | |||
There was a problem hiding this comment.
not sure if intentional but you added a toString() here
There was a problem hiding this comment.
Yea I think that's a bug fix.
| @@ -264,9 +260,9 @@ export function initInput( | |||
| // Only assign the checked attribute if it is defined. This saves | |||
| // a DOM write when controlling the checked attribute isn't needed | |||
| // (text inputs, submit/reset) | |||
| if (props.defaultChecked != null) { | |||
| if (defaultChecked != null) { | |||
| node.defaultChecked = !node.defaultChecked; | |||
There was a problem hiding this comment.
(what does this extra assignment even do? seems like it should have no effect:
The checked content attribute is a boolean attribute that gives the default checkedness of the input element. When the checked content attribute is added, if the control does not have dirty checkedness, the user agent must set the checkedness of the element to true; when the checked content attribute is removed, if the control does not have dirty checkedness, the user agent must set the checkedness of the element to false.
)
There was a problem hiding this comment.
Need investigation if it's a browser quirk. I could imagine browsers storing attribute and property values separately and missing a case where they should be updated and therefore not aligning. But not sure why this was added.
| domElement.setAttribute('name', name); | ||
| } else { | ||
| domElement.removeAttribute('name'); | ||
| } |
There was a problem hiding this comment.
setProp here too? and/or is it faster to assign to .name which should be equivalent?
There was a problem hiding this comment.
Same thing. I really just want to set domElement.name = name and I also want to make clear that setProp doesn't do anything complicated for this value.
| @@ -1265,12 +1309,12 @@ export function updateProperties( | |||
| ) { | |||
| switch (propKey) { | |||
| case 'checked': { | |||
| const checked = nextProps.defaultChecked; | |||
There was a problem hiding this comment.
wait what we read props.defaultChecked and assign it to node.checked?
There was a problem hiding this comment.
This is the case when 'checked' is deleted. It's the same as setting it to null below.
| @@ -1364,28 +1449,59 @@ export function updateProperties( | |||
| didWarnControlledToUncontrolled = true; | |||
| } | |||
| } | |||
|
|
|||
| // Update checked *before* name. | |||
There was a problem hiding this comment.
I believe this isn't the same as the behavior before. If you update from
<input type="checkbox" name="a" checked={true} />to
<input type="radio" name="b" checked={false} />then I believe now it will uncheck other a radios as well as other b radios (!) but the old code didn't. I think if only name and checked change then you didn't regress, but also I'm unsure how much it matters given the other bug I noticed where this handling doesn't really reliably work for name and checked anyway…?
The best strategy I could come up with that seemed obviously correct in all cases was to unset name before changing either type/checked and then set it back after. (You could also set checked to false but I was having trouble thinking about how that affects the dirty checkedness flag and interplay with the attribute.)
There was a problem hiding this comment.
Before it would always set checked before the loop. It would sometimes set it again inside the loop, before or after "name" depending on enumeration order of the props.
Now it will consistently always set checked first and name later.
Are you saying I should set type after the loop too?
I guess we just need to fix it properly.
There was a problem hiding this comment.
Basically it seems you want to avoid any intermediate states that have simultaneously node.type === 'radio' and node.checked === true and node.name nonempty. Which is tricky because you could be transitioning either from that or to that. I don't think any of the six possible (type, name, checked) permutations is always correct. So either you need to be really fancy about what values are changing and set them in different orders as a result, or you need to unset one of the properties, update the other two, then set the first.
There was a problem hiding this comment.
We already do that here:
I think maybe this one is just here because of the weird case where checked is also set in the props - unlike value. It seems like maybe we should also use the special cases in updateInput and not update checked nor type in the loop.
There was a problem hiding this comment.
Oh, great. Yeah if we only set type and checked within that block then I think it's chill.
…led components (#26596) Since `props.x` is a possibly megamorphic access, it can be slow to access and trigger recompilation. When we are looping over the props and pattern matching every key, anyway, we've already done this work. We can just reuse the same value by stashing it outside the loop in the stack. This only makes sense for updates in diffInCommitPhase since otherwise we don't have the full set of props in that loop. We also have to be careful not to skip over equal values since we need to extract them anyway. DiffTrain build for [6b90976](6b90976)
… controlled components (facebook#26596)" This reverts commit 6b90976.
I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state. When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`: https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69 Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null. This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled. Related to that, there was also another issue observed in #26596 (comment) and #26588 We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com>
I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state. When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`: https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69 Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null. This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled. Related to that, there was also another issue observed in #26596 (comment) and #26588 We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com> DiffTrain build for [1f248bd](1f248bd)
, facebook#26595, facebook#26596, facebook#26627 - Refactor some controlled component stuff (facebook#26573) - Don't update textarea defaultValue and input checked unnecessarily (facebook#26580) - Diff properties in the commit phase instead of generating an update payload (facebook#26583) - Move validation of text nesting into ReactDOMComponent (facebook#26594) - Remove initOption special case (facebook#26595) - Use already extracted values instead of reading off props for controlled components (facebook#26596) - Fix input tracking bug (facebook#26627)
- Refactor some controlled component stuff (#26573) - Don't update textarea defaultValue and input checked unnecessarily (#26580) - Diff properties in the commit phase instead of generating an update payload (#26583) - Move validation of text nesting into ReactDOMComponent (#26594) - Remove initOption special case (#26595) - Use already extracted values instead of reading off props for controlled components (#26596) - Fix input tracking bug (#26627) DiffTrain build for [ded4a78](ded4a78)
- Refactor some controlled component stuff (#26573) - Don't update textarea defaultValue and input checked unnecessarily (#26580) - Diff properties in the commit phase instead of generating an update payload (#26583) - Move validation of text nesting into ReactDOMComponent (#26594) - Remove initOption special case (#26595) - Use already extracted values instead of reading off props for controlled components (#26596) - Fix input tracking bug (#26627)
I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state. When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`: https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69 Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null. This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled. Related to that, there was also another issue observed in #26596 (comment) and #26588 We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com>
I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state. When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`: https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69 Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null. This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled. Related to that, there was also another issue observed in facebook/react#26596 (comment) and facebook/react#26588 We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com> DiffTrain build for [1f248bdd7199979b050e4040ceecfe72dd977fd1](facebook/react@1f248bd)
…led components (facebook#26596) Since `props.x` is a possibly megamorphic access, it can be slow to access and trigger recompilation. When we are looping over the props and pattern matching every key, anyway, we've already done this work. We can just reuse the same value by stashing it outside the loop in the stack. This only makes sense for updates in diffInCommitPhase since otherwise we don't have the full set of props in that loop. We also have to be careful not to skip over equal values since we need to extract them anyway.
) I accidentally made a behavior change in the refactor. It turns out that when switching off `checked` to an uncontrolled component, we used to revert to the concept of "initialChecked" which used to be stored on state. When there's a diff to this computed prop and the value of props.checked is null, then we end up in a case where it sets `checked` to `initialChecked`: https://github.com/facebook/react/blob/5cbe6258bc436b1683080a6d978c27849f1d9a22/packages/react-dom-bindings/src/client/ReactDOMInput.js#L69 Since we never changed `initialChecked` and it's not relevant if non-null `checked` changes value, the only way this "change" could trigger was if we move from having `checked` to having null. This wasn't really consistent with how `value` works, where we instead leave the current value in place regardless. So this is a "bug fix" that changes `checked` to be consistent with `value` and just leave the current value in place. This case should already have a warning in it regardless since it's going from controlled to uncontrolled. Related to that, there was also another issue observed in facebook#26596 (comment) and facebook#26588 We need to atomically apply mutations on radio buttons. I fixed this by setting the name to empty before doing mutations to value/checked/type in updateInput, and then set the name to whatever it should be. Setting the name is what ends up atomically applying the changes. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com>
…led components (#26596) Since `props.x` is a possibly megamorphic access, it can be slow to access and trigger recompilation. When we are looping over the props and pattern matching every key, anyway, we've already done this work. We can just reuse the same value by stashing it outside the loop in the stack. This only makes sense for updates in diffInCommitPhase since otherwise we don't have the full set of props in that loop. We also have to be careful not to skip over equal values since we need to extract them anyway. DiffTrain build for commit 6b90976.
Since
props.xis a possibly megamorphic access, it can be slow to access and trigger recompilation.When we are looping over the props and pattern matching every key, anyway, we've already done this work. We can just reuse the same value by stashing it outside the loop in the stack.
This only makes sense for updates in diffInCommitPhase since otherwise we don't have the full set of props in that loop.
We also have to be careful not to skip over equal values since we need to extract them anyway.