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

InputControl: avoid uncontrolled to controlled warning #47250

Merged
merged 2 commits into from
Jan 20, 2023

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Jan 18, 2023

What?

Make sure that the underlying <input /> in InputControl doesn't switch inadvertently from uncontrolled to controlled mode.

Why?

While working on #45982 and on #47246, I noticed that the "uncontrolled to controlled" warning was being logged even when it shouldn't have.

After some investigation with @mirka, we realized that InputControl always passes an explicit value prop to the underlying <input /> element, because of the component's internal state reducer.

This causes the "uncontrolled to controlled" warning to be logged when value of the input goes from empty to any value.

I will also note that this error doesn't probably get logged when using the editor because in most cases a fallback to an empty string is already provided by the component using InputControl (or any related component).

How?

Since React interprets an undefined value as a sign of an uncontrolled component, the fix here is to pass an empty string to <input /> instead of undefined (this is achieved via the ?? '' code).

Testing Instructions

About the error message:

  • Checkout this PR.
  • Remove the fix that this PR applies to the packages/components/src/input-control/input-field.tsx file
  • Load the temporary "Uncontrolled" Storybook example. Input a value in the input field.
  • Observe how the "uncontrolled to controlled" error message gets logged to console
  • Apply the fix from this PR to the packages/components/src/input-control/input-field.tsx file
  • Try again interacting with the "Uncontrolled" Storybook example, make sure that the error is not logged to console

Check that the component behaves as expected:

  • In Storybook for InputControl and its derivatives (NumberControl, UnitControl, RangeControl, ....)
  • In the editor, especially in the sidebar

Screenshots or screencast

Screencast of the error message logged to console that happens without the fix from this PR:

number-control-controlled-warning.mp4

@ciampo ciampo self-assigned this Jan 18, 2023
@ciampo ciampo marked this pull request as ready for review January 18, 2023 16:46
@ciampo ciampo requested a review from ajitbohra as a code owner January 18, 2023 16:46
@ciampo ciampo added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Jan 18, 2023
@ciampo
Copy link
Contributor Author

ciampo commented Jan 18, 2023

The test failures don't seem related to this PR, especially since they seem to happen across many other PRs as well.

Comment on lines 66 to 68
// Check if this was broken and at what point
// in particular on commit actions, when `min` is not `undefined`
export const Uncontrolled: ComponentStory< typeof NumberControl > = ( {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please ignore the code comment, it's a left over from a copy-paste.

The whole Storybook example will be removed before merging, I'm just leaving around for ease of testing

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

While reviewing this PR, I also briefly thought about whether there are more appropriate places to put the fallback. For example, at the prop destructure point or the useInputControlStateReducer() call point. But I still think the place we currently have it is the most appropriate, because the string fallback is only necessary where our value interfaces with React <input> 👍

@ciampo ciampo force-pushed the feat/input-control-fix-uncontrolled-warning branch from 3dfdc96 to 6dbc4c0 Compare January 20, 2023 16:11
@ciampo ciampo enabled auto-merge (squash) January 20, 2023 16:43
@ciampo ciampo merged commit 2bf7068 into trunk Jan 20, 2023
@ciampo ciampo deleted the feat/input-control-fix-uncontrolled-warning branch January 20, 2023 16:50
@github-actions github-actions bot added this to the Gutenberg 15.1 milestone Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants