Refactor some controlled component stuff #26573

sebmarkbage · 2023-04-08T04:26:27Z

This is mainly renaming some stuff. The behavior change is hasOwnProperty to nullish check.

I had a bigger refactor that was a dead-end but might as well land this part and see if I can pick it up later.

These used to be wrapper components which is where the name came from but that's not relevant anymore.

…emantics

react-sizebot · 2023-04-08T04:31:01Z

Comparing: 657698e...91f7014

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	165.16 kB	164.55 kB	=	51.86 kB	51.70 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	166.74 kB	166.13 kB	=	52.37 kB	52.18 kB
facebook-www/ReactDOM-prod.classic.js	=	555.16 kB	553.99 kB	=	98.41 kB	98.16 kB
facebook-www/ReactDOM-prod.modern.js	=	539.00 kB	537.83 kB	=	95.74 kB	95.49 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
facebook-www/ReactDOM-profiling.classic.js	=	585.66 kB	584.49 kB	=	102.96 kB	102.72 kB
facebook-www/ReactDOM-profiling.modern.js	=	569.44 kB	568.27 kB	=	100.24 kB	99.99 kB
facebook-www/ReactDOMTesting-prod.classic.js	=	569.87 kB	568.69 kB	=	102.06 kB	101.82 kB
facebook-www/ReactDOMTesting-prod.modern.js	=	555.54 kB	554.37 kB	=	99.91 kB	99.67 kB
facebook-www/ReactDOM-prod.classic.js	=	555.16 kB	553.99 kB	=	98.41 kB	98.16 kB
facebook-www/ReactDOM-prod.modern.js	=	539.00 kB	537.83 kB	=	95.74 kB	95.49 kB
oss-experimental/react-dom/umd/react-dom.profiling.min.js	=	175.63 kB	175.03 kB	=	55.07 kB	54.90 kB
oss-experimental/react-dom/cjs/react-dom.profiling.min.js	=	176.37 kB	175.76 kB	=	54.79 kB	54.63 kB
oss-stable/react-dom/umd/react-dom.profiling.min.js	=	174.05 kB	173.45 kB	=	54.61 kB	54.41 kB
oss-stable-semver/react-dom/umd/react-dom.profiling.min.js	=	173.98 kB	173.37 kB	=	54.59 kB	54.39 kB
oss-stable/react-dom/cjs/react-dom.profiling.min.js	=	174.79 kB	174.18 kB	=	54.28 kB	54.12 kB
oss-stable-semver/react-dom/cjs/react-dom.profiling.min.js	=	174.72 kB	174.11 kB	=	54.25 kB	54.09 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.production.min.js	=	172.95 kB	172.34 kB	=	54.71 kB	54.52 kB
oss-experimental/react-dom/umd/react-dom.production.min.js	=	166.65 kB	166.04 kB	=	52.78 kB	52.61 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	166.74 kB	166.13 kB	=	52.37 kB	52.18 kB
oss-stable/react-dom/umd/react-dom.production.min.js	=	165.06 kB	164.46 kB	=	52.29 kB	52.10 kB
oss-stable-semver/react-dom/umd/react-dom.production.min.js	=	164.99 kB	164.39 kB	=	52.26 kB	52.08 kB
oss-stable/react-dom/cjs/react-dom.production.min.js	=	165.16 kB	164.55 kB	=	51.86 kB	51.70 kB
oss-stable-semver/react-dom/cjs/react-dom.production.min.js	=	165.09 kB	164.48 kB	=	51.84 kB	51.68 kB

Generated by 🚫 dangerJS against 91f7014

sebmarkbage · 2023-04-08T04:32:21Z

packages/react-dom-bindings/src/client/ReactDOMComponent.js

      // 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));
+      initInput(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.


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.

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.

sophiebits

Is there a bug in the existing code when removing defaultValue? This fails for me:

  it('should remove previous `defaultValue`', () => {
    const node = ReactDOM.render(
      <input type="text" defaultValue="0" />,
      container,
    );

    expect(node.value).toBe('0');
    expect(node.defaultValue).toBe('0');

    ReactDOM.render(<input type="text" />, container);
    expect(node.defaultValue).toBe('');
  });

 FAIL  packages/react-dom/src/__tests__/ReactDOMInput-test.js (7.948 s)
  ● ReactDOMInput › should remove previous `defaultValue`

    Expected: ""
    Received: "0"

      461 |
      462 |     ReactDOM.render(<input type="text" />, container);
    > 463 |     expect(node.defaultValue).toBe('');
          |                               ^
      464 |   });
      465 |

sebmarkbage · 2023-04-08T17:23:56Z

Tbh, I feel like there are a lot of subtle semantics that are wrong here. However, for that particular scenario I could see how that would be expected given the "attribute syncing" since defaultValue is the same as the attribute, it means that since the internal state of the value is still the stateful value, and if that is sync:ed back to the attribute then the defaultValue becomes that too.

If gated on the disableInputAttributeSyncing it does seems like that should work though.

sophiebits · 2023-04-08T17:30:44Z

since defaultValue is the same as the attribute, it means that since the internal state of the value is still the stateful value, and if that is sync:ed back to the attribute then the defaultValue becomes that too

Not sure I understand what you mean here. FWIW updating the defaultValue prop does set the attribute regardless of the feature flag so I expected that removing the prop would behave the same way:

react/packages/react-dom/src/__tests__/ReactDOMInput-test.js

Line 449 in 657698e

expect(node.defaultValue).toBe('1');

sebmarkbage · 2023-04-08T17:52:47Z

I see, yea, if it has never transitioned to having a controlled or stateful "value" then it should. I originally read it as transitioning from controlled "value" in which case you would have a lingering value which would then need to be reflected in the attribute under current semantics.

sebmarkbage · 2023-04-08T19:21:19Z

It seems like the implementation is not consistent about hasOwnProperty, undefined and null. For example, we treat null as something that should fall back to the defaultValue if it's available, but in other cases, we treat only undefined as missing. This leads to a combination of cases that only happen with certain types of transitions. However it mostly comes with a warning if you specify null.

This is problematic because both on the server and diffing, we actually filter out this difference early.

https://github.com/facebook/react/blob/main/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js#L1029
https://github.com/facebook/react/blob/main/packages/react-dom-bindings/src/client/ReactDOMComponent.js#L1126-L1128

Basically the rule is that they should always be treated the same, regardless of which prop it is. However, there's provably code that depends on this distinction but since that's further below these checks, it's also provably not going to be consistent. E.g. changing from null to undefined won't have the same behavior as defining undefined in the first place.

There's a subtle difference when checked turns to null we now use the defaultChecked prop instead of initialChecked.

This lines up better with how this is done on the server already.

… same props

Check whether something is controlled from props in the event system.

We don't use it anymore.

It can be derived from props.

wasMultiple can be derived from props instead

sebmarkbage · 2023-04-09T20:47:42Z

Ok, I did some further refactoring. I removed all the _wrapperState to show that they're not actually used to store any meaningful state. They're just derived from initial props. Since defaultValue does update.

I also fixed it so that removing or switching defaultValue to null or empty does update on input and textarea. Since textarea doesn't have an attribute but the child content, I just update to an empty string for this case.

I did this by changing the order defaultValue and value are assigned, which I'm a bit worried about will lead to something unpredictable.

sebmarkbage · 2023-04-09T20:49:03Z

packages/react-dom-bindings/src/client/ReactDOMTextarea.js

+    if (newValue !== node.value) {
+      node.value = newValue;
+    }
+    // TOOO: This should respect disableInputAttributeSyncing flag.


@gaearon This doesn't update an attribute but it does update the internal state so it falls under the same category of issues.

sebmarkbage · 2023-04-09T20:58:02Z

packages/react-dom/src/__tests__/ReactDOMInput-test.js

-      } else {
-        expect(input.getAttribute('value')).toBe('first');
-      }
+      expect(input.getAttribute('value')).toBe(null);


This is considered a bug fix.

For some types, this already resets the value, so if we also have a value we want to set that after. So this moves to updating defaultValue before value.

This doesn't have an attribute to be removed so we just reset it to empty string.

Since the init is now just validation I rename those methods appropriately. Since postInit is now just where initialization happens, that's just init.

This is mainly renaming some stuff. The behavior change is hasOwnProperty to nullish check. I had a bigger refactor that was a dead-end but might as well land this part and see if I can pick it up later. DiffTrain build for [e5146cb](e5146cb)

sophiebits

lgtm

sophiebits · 2023-04-10T01:33:57Z

packages/react-dom-bindings/src/client/ReactDOMInput.js

+  const defaultChecked =
+    props.checked != null ? props.checked : props.defaultChecked;


seems a little confusing that your defaultValue and defaultChecked are not parallel (the latter here incorporates props.checked) but I guess it's fine

sophiebits · 2023-04-10T01:41:55Z

packages/react-dom-bindings/src/events/plugins/ChangeEventPlugin.js

+    // These props aren't necessarily the most current but we warn for changing
+    // between controlled and uncontrolled, so it doesn't matter and the previous
+    // code was also broken for changes.
+    const props = targetInst.memoizedProps;


nit: pass isControlled (props.value != null) here instead of having it in the callee? since the props can be stale, seems better to localize the calculations to the same place where we know they are stale

sophiebits · 2023-04-10T01:42:54Z

packages/react-dom-bindings/src/client/ReactDOMInput.js

-  const defaultValue = props.defaultValue == null ? '' : props.defaultValue;
-  const initialChecked =
-    props.checked != null ? props.checked : props.defaultChecked;
-  node._wrapperState = {


…26580) In #26573 I changed it so that textareas get their defaultValue reset if you don't specify one. However, the way that was implemented, it always set it for any update even if it hasn't changed. We have a test for that, but that test only works if no properties update at all so that no update was scheduled. This fixes the test so that it updates some unrelated prop. I also found a test for `checked` that needed a similar fix. Interestingly, we don't do this deduping for `defaultValue` or `defaultChecked` on inputs and there's no test for that.

…26580) In #26573 I changed it so that textareas get their defaultValue reset if you don't specify one. However, the way that was implemented, it always set it for any update even if it hasn't changed. We have a test for that, but that test only works if no properties update at all so that no update was scheduled. This fixes the test so that it updates some unrelated prop. I also found a test for `checked` that needed a similar fix. Interestingly, we don't do this deduping for `defaultValue` or `defaultChecked` on inputs and there's no test for that. DiffTrain build for [9a9da77](9a9da77)

Summary: This sync includes the following changes: - **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([#26599](facebook/react#26599)) //<Andrew Clark>// - **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([#26597](facebook/react#26597)) //<Andrew Clark>// - **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent ([#26594](facebook/react#26594)) //<Sebastian Markbåge>// - **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([#26583](facebook/react#26583)) //<Sebastian Markbåge>// - **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([#26593](facebook/react#26593)) //<Josh Story>// - **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([#26592](facebook/react#26592)) //<Josh Story>// - **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([#26591](facebook/react#26591)) //<Josh Story>// - **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([#26590](facebook/react#26590)) //<Josh Story>// - **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([#26589](facebook/react#26589)) //<Josh Story>// - **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([#26552](facebook/react#26552)) //<Andrew Clark>// - **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([#26580](facebook/react#26580)) //<Sebastian Markbåge>// - **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([#26573](facebook/react#26573)) //<Sebastian Markbåge>// - **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([#26568](facebook/react#26568)) //<Josh Story>// - **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([#26561](facebook/react#26561)) //<Andrew Clark>// - **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([#26558](facebook/react#26558)) //<Sebastian Markbåge>// - **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([#26553](facebook/react#26553)) //<Sophie Alpert>// - **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([#26551](facebook/react#26551)) //<Sebastian Markbåge>// - **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([#26534](facebook/react#26534)) //<Mohammad Ghorbani>// - **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([#26546](facebook/react#26546)) //<Sebastian Markbåge>// - **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([#26236](facebook/react#26236))" ([#26528](facebook/react#26528)) //<Jan Kassens>// - **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([#26516](facebook/react#26516)) //<Rubén Norte>// - **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([#26526](facebook/react#26526)) //<Josh Story>// - **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([#26535](facebook/react#26535)) //<Hans Otto Wirtz>// - **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([#26532](facebook/react#26532)) //<Andrew Clark>// - **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([#26531](facebook/react#26531)) //<Andrew Clark>// - **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([#26512](facebook/react#26512)) //<Andrew Clark>// - **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([#26521](facebook/react#26521)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions ca01f35...58742c2 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44872333 fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c

This reverts commit e5146cb.

This is mainly renaming some stuff. The behavior change is hasOwnProperty to nullish check. I had a bigger refactor that was a dead-end but might as well land this part and see if I can pick it up later.

This is mainly renaming some stuff. The behavior change is hasOwnProperty to nullish check. I had a bigger refactor that was a dead-end but might as well land this part and see if I can pick it up later. DiffTrain build for [e48e771](e48e771)

, 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)

Summary: This sync includes the following changes: - **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([facebook#26599](facebook/react#26599)) //<Andrew Clark>// - **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([facebook#26597](facebook/react#26597)) //<Andrew Clark>// - **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent ([facebook#26594](facebook/react#26594)) //<Sebastian Markbåge>// - **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([facebook#26583](facebook/react#26583)) //<Sebastian Markbåge>// - **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([facebook#26593](facebook/react#26593)) //<Josh Story>// - **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([facebook#26592](facebook/react#26592)) //<Josh Story>// - **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([facebook#26591](facebook/react#26591)) //<Josh Story>// - **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([facebook#26590](facebook/react#26590)) //<Josh Story>// - **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([facebook#26589](facebook/react#26589)) //<Josh Story>// - **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([facebook#26552](facebook/react#26552)) //<Andrew Clark>// - **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([facebook#26580](facebook/react#26580)) //<Sebastian Markbåge>// - **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([facebook#26573](facebook/react#26573)) //<Sebastian Markbåge>// - **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([facebook#26568](facebook/react#26568)) //<Josh Story>// - **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([facebook#26561](facebook/react#26561)) //<Andrew Clark>// - **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([facebook#26558](facebook/react#26558)) //<Sebastian Markbåge>// - **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([facebook#26553](facebook/react#26553)) //<Sophie Alpert>// - **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([facebook#26551](facebook/react#26551)) //<Sebastian Markbåge>// - **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([facebook#26534](facebook/react#26534)) //<Mohammad Ghorbani>// - **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([facebook#26546](facebook/react#26546)) //<Sebastian Markbåge>// - **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([facebook#26236](facebook/react#26236))" ([facebook#26528](facebook/react#26528)) //<Jan Kassens>// - **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([facebook#26516](facebook/react#26516)) //<Rubén Norte>// - **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([facebook#26526](facebook/react#26526)) //<Josh Story>// - **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([facebook#26535](facebook/react#26535)) //<Hans Otto Wirtz>// - **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([facebook#26532](facebook/react#26532)) //<Andrew Clark>// - **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([facebook#26531](facebook/react#26531)) //<Andrew Clark>// - **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([facebook#26512](facebook/react#26512)) //<Andrew Clark>// - **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([facebook#26521](facebook/react#26521)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions ca01f35...58742c2 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44872333 fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c

This is mainly renaming some stuff. The behavior change is hasOwnProperty to nullish check. I had a bigger refactor that was a dead-end but might as well land this part and see if I can pick it up later.

…acebook#26580) In facebook#26573 I changed it so that textareas get their defaultValue reset if you don't specify one. However, the way that was implemented, it always set it for any update even if it hasn't changed. We have a test for that, but that test only works if no properties update at all so that no update was scheduled. This fixes the test so that it updates some unrelated prop. I also found a test for `checked` that needed a similar fix. Interestingly, we don't do this deduping for `defaultValue` or `defaultChecked` on inputs and there's no test for that.

This is mainly renaming some stuff. The behavior change is hasOwnProperty to nullish check. I had a bigger refactor that was a dead-end but might as well land this part and see if I can pick it up later. DiffTrain build for commit e5146cb.

…26580) In #26573 I changed it so that textareas get their defaultValue reset if you don't specify one. However, the way that was implemented, it always set it for any update even if it hasn't changed. We have a test for that, but that test only works if no properties update at all so that no update was scheduled. This fixes the test so that it updates some unrelated prop. I also found a test for `checked` that needed a similar fix. Interestingly, we don't do this deduping for `defaultValue` or `defaultChecked` on inputs and there's no test for that. DiffTrain build for commit 9a9da77.

sebmarkbage added 2 commits April 8, 2023 00:25

Rename controlled component methods

087f5e9

These used to be wrapper components which is where the name came from but that's not relevant anymore.

Use nullish checks instead of hasOwnProperty which is generally our s…

f57beb1

…emantics

sebmarkbage requested review from sophiebits and gaearon April 8, 2023 04:26

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 8, 2023

sebmarkbage commented Apr 8, 2023

View reviewed changes

sophiebits approved these changes Apr 8, 2023

View reviewed changes

sebmarkbage added 7 commits April 9, 2023 15:25

Move init and postInit together to show that this doesn't matter

0e0af20

There's a subtle difference when checked turns to null we now use the defaultChecked prop instead of initialChecked.

Move checkControlledValueProps to ReactDOMComponent

0c9c688

This lines up better with how this is done on the server already.

Fix tests

e6f26c3

setDefaultValue is always guarded by a null check

a0a47e5

Since we just initialized the initial state, we can infer it from the…

1ad8392

… same props

Move controlled warning to be done by last/next props

17abdbe

Check whether something is controlled from props in the event system.

Delete wrapperState

b3e97c5

We don't use it anymore.

sebmarkbage force-pushed the refactorcontrolled branch from 2a51928 to 5e01aee Compare April 9, 2023 20:40

sebmarkbage added 2 commits April 9, 2023 16:42

Textarea only initializes the wrapper state to read it at the same time

63b40e5

It can be derived from props.

Remove wrapperState from select

18a5a9f

wasMultiple can be derived from props instead

sebmarkbage force-pushed the refactorcontrolled branch from 5e01aee to 2011bfd Compare April 9, 2023 20:45

sebmarkbage commented Apr 9, 2023

View reviewed changes

sebmarkbage force-pushed the refactorcontrolled branch from 2011bfd to 5076b3a Compare April 9, 2023 20:57

sebmarkbage commented Apr 9, 2023

View reviewed changes

sebmarkbage added 2 commits April 9, 2023 17:02

Delete value attribute when defaultValue sets back to null/missing

2019ddc

For some types, this already resets the value, so if we also have a value we want to set that after. So this moves to updating defaultValue before value.

Reset textarea to empty after defaultValue is removed or null

b263fa3

This doesn't have an attribute to be removed so we just reset it to empty string.

Rename initX to validateXProps and postInitX to initX

91f7014

Since the init is now just validation I rename those methods appropriately. Since postInit is now just where initialization happens, that's just init.

sebmarkbage force-pushed the refactorcontrolled branch from 5076b3a to 91f7014 Compare April 9, 2023 21:02

sebmarkbage merged commit e5146cb into facebook:main Apr 9, 2023

sebmarkbage mentioned this pull request Apr 10, 2023

Don't update textarea defaultValue and input checked unnecessarily #26580

Merged

sophiebits reviewed Apr 10, 2023

View reviewed changes

kassens added a commit to kassens/react that referenced this pull request Apr 17, 2023

Revert "Refactor some controlled component stuff (facebook#26573)"

481793a

This reverts commit e5146cb.

kassens added a commit that referenced this pull request Apr 20, 2023

Revert "Refactor some controlled component stuff (#26573)"

d1cccc0

This reverts commit e5146cb.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor some controlled component stuff #26573

Refactor some controlled component stuff #26573

sebmarkbage commented Apr 8, 2023

react-sizebot commented Apr 8, 2023 •

edited

Loading

sebmarkbage Apr 8, 2023

sebmarkbage Apr 8, 2023

sophiebits left a comment

sebmarkbage commented Apr 8, 2023

sophiebits commented Apr 8, 2023

sebmarkbage commented Apr 8, 2023 •

edited

Loading

sebmarkbage commented Apr 8, 2023 •

edited

Loading

sebmarkbage commented Apr 9, 2023 •

edited

Loading

sebmarkbage Apr 9, 2023

sebmarkbage Apr 9, 2023

sophiebits left a comment

sophiebits Apr 10, 2023

sophiebits Apr 10, 2023

sophiebits Apr 10, 2023

		const defaultChecked =
		props.checked != null ? props.checked : props.defaultChecked;

Refactor some controlled component stuff #26573

Refactor some controlled component stuff #26573

Conversation

sebmarkbage commented Apr 8, 2023

react-sizebot commented Apr 8, 2023 • edited Loading

Critical size changes

Significant size changes

sebmarkbage Apr 8, 2023

Choose a reason for hiding this comment

sebmarkbage Apr 8, 2023

Choose a reason for hiding this comment

sophiebits left a comment

Choose a reason for hiding this comment

sebmarkbage commented Apr 8, 2023

sophiebits commented Apr 8, 2023

sebmarkbage commented Apr 8, 2023 • edited Loading

sebmarkbage commented Apr 8, 2023 • edited Loading

sebmarkbage commented Apr 9, 2023 • edited Loading

sebmarkbage Apr 9, 2023

Choose a reason for hiding this comment

sebmarkbage Apr 9, 2023

Choose a reason for hiding this comment

sophiebits left a comment

Choose a reason for hiding this comment

sophiebits Apr 10, 2023

Choose a reason for hiding this comment

sophiebits Apr 10, 2023

Choose a reason for hiding this comment

sophiebits Apr 10, 2023

Choose a reason for hiding this comment

react-sizebot commented Apr 8, 2023 •

edited

Loading

sebmarkbage commented Apr 8, 2023 •

edited

Loading

sebmarkbage commented Apr 8, 2023 •

edited

Loading

sebmarkbage commented Apr 9, 2023 •

edited

Loading