Use controlled state with value #3028

asudoh · 2019-06-12T08:48:56Z

This change turns <NumberInput> to "controlled mode", if value prop exists. Such "controlled mode" switch is behind useControlledStateWithEventListener feature flag for now to given it's a breaking change.

Also this change introduces readOnly prop to <NumberInput> with corresponding style.

Refs #2489.

Changelog

New

A mechanism to turn <NumberInput> to "controlled mode", if value prop exists. Such "controlled mode" switch is behind useControlledStateWithEventListener feature flag for now to given it's a breaking change.
readOnly prop to <NumberInput> with corresponding style.

Changed

0 default value logic moved out from defaultProps for the sake of defaultValue support.
onChange signature to include the new value to support controlled mode. This is behind useControlledStateWithEventListener feature flag for now to given it's a breaking change.
onChange prop type checking logic to make it required when value exists and readOnly doesn't.

Testing / Reviewing

Testing should make sure number input is not broken.

This change turns `<NumberInput>` to "controlled mode", if `value` prop exists. Such "controlled mode" switch is behind `useControlledStateWithEventListener` feature flag for now to given it's a breaking change. Also this change introduces `readOnly` prop to `<NumberInput>` with corresponding style. Refs carbon-design-system#2489.

netlify · 2019-06-12T08:56:15Z

Deploy preview for the-carbon-components ready!

Built with commit 4a48fc2

https://deploy-preview-3028--the-carbon-components.netlify.com

netlify · 2019-06-12T08:57:04Z

Deploy preview for carbon-components-react ready!

Built with commit 4a48fc2

https://deploy-preview-3028--carbon-components-react.netlify.com

netlify · 2019-06-12T08:57:26Z

Deploy preview for carbon-elements ready!

Built with commit 4a48fc2

https://deploy-preview-3028--carbon-elements.netlify.com

joshblack · 2019-06-12T15:44:14Z

Why is a feature flag required for this?

asudoh · 2019-06-13T02:20:57Z

@joshblack Good question - In older version, having value without onChange does not make the component read-only state that is different from what the newer version does. Given that it's a breaking change and needs to be roped off until a major change comes.

vpicone · 2019-06-14T18:04:41Z

packages/react/src/prop-types/requiresIfValueExists.js

+ * @param {Function} propType The original prop type checker.
+ * @returns {Function} The new prop type checker for `onChange` that makes it required if `value` exists and `readOnly` does not exist.
+ */
+export default function requiresIfValueExists(propType) {


This is neat, just a small grammar thing. I'd name it requiredIfValueExists

packages/react/src/components/NumberInput/NumberInput.js

vpicone · 2019-06-14T22:14:04Z

packages/react/src/components/NumberInput/NumberInput.js

@@ -38,7 +40,11 @@ const capMax = (max, value) =>
 class NumberInput extends Component {
  constructor(props) {
    super(props);
-    let value = props.value;
+    let value =
+      useControlledStateWithValue || props.value === undefined


Important: The defaultValue should be completely ignored if a value prop is passed in. It's only purpose is to supply an initial state for an otherwise uncontrolled component. With your current implementation, the defaultValue will override a value prop if both were passed in.

A few more things:

I think we can clean up this this logic with the capMin helper

We should refactor to avoid relying on undefined checks and avoid setting variables to undefined.

This refactoring means we can set a proper defaultProp for the devaultValue (0).

We should also add a controlledMode instance field to consolidates the flag checks. In my opinion, an instance field is more appropriate than state since we never intend on changing the property during the life cycle of the component.

constructor(props) { super(props); const { value: valueProp, defaultValue, min } = props; this.controlledMode = useControlledStateWithValue && valueProp !== undefined; const value = this.controlledMode ? valueProp : defaultValue; this.state = { value: capMin(min, value) }; } // Add to defaultProps // static defaultProps = { // defaultValue: 0 // };

I have a sandbox with a demo of the constructor

@emyarod this behavior might be related to #3013

packages/react/src/components/NumberInput/NumberInput.js

vpicone · 2019-06-14T22:18:20Z

packages/react/src/components/NumberInput/NumberInput.js

@@ -254,7 +281,11 @@ class NumberInput extends Component {
      min,
      step,
      onChange: this.handleChange,
-      value: this.state.value,
+      value:
+        useControlledStateWithValue && value !== undefined


This is an example of a spot where we'd use the instance field.

packages/react/src/components/NumberInput/NumberInput.js

vpicone · 2019-06-17T13:44:19Z

packages/react/src/components/NumberInput/NumberInput.js

+    const result = this.props.value !== undefined;
+    if (lastIsControlled !== undefined && lastIsControlled !== result) {
+      warning(
+        'A component is changing an uncontrolled `NumberInput` to be controlled. ' +


The fact that we're trying to emulate 2 react errors feels like a code smell to me. How many errors, edge cases are we missing? This feels like a strong indication we're going the wrong direction with how we're attempting to manage the state of this component. We'd need to replicate this error in each component using this pattern for each piece of managed state?

packages/react/src/internal/FeatureFlags.js

vpicone

In the must have functionality of my issue regarding this topic one of the two points was "The component should never be both controlled and uncontrolled. When a derived state value is also updated by setState calls, there isn't a single source of truth for the data.

I was a fan of the hybrid approach when it emulated separate presentational and stateful components. This implementation feels really different than the initial goal of separating controlled and uncontrolled components. This hybrid approach attempts to support controlled and uncontrolled throughout the lifecycle of the component and I fear could introduce a lot of complexity in trying to sync the states between these two disparate sources of state changes.

While we can warn developers, we should also be wary of how this will scale to components with multiple forms of potentially controlled state. Managing errors like this within components would be a lot of maintenance overhead.

vpicone · 2019-06-17T21:45:28Z

Here is an example implementation that persists controlled/uncontrolled mode of the component so that react can properly generate the warnings for us:

https://codesandbox.io/s/focused-worker-hbh7l

asudoh

Thanks @vpicone for your review, as we discussed offline, I made the following updates:

Determine the component should be controlled/uncontrolled at creation time and stuck to it for the lifetime of the component instance (Will rectify this decision whenever we find application would have a problem with this)
Added more documentation of what the new feature flag does, including the changed argument list of the event handlers
Updated a comment in gDSFP() that it'll be no-op when the feature flag is turned on

Also as discussed offline, let me know if you anticipate any more documentation. Thanks!

packages/react/src/components/NumberInput/NumberInput.js

packages/react/src/internal/FeatureFlags.js

vpicone · 2019-06-18T01:41:16Z

packages/react/src/internal/FeatureFlags.js

+ * * _With_ this feature flag, the event handler has `onChange(event, { value, direction })` where:
+ *   * `event` is the (React) raw event
+ *   * `value` is the new value
+ *   * `direction` tells you the button you hit is up button or down button


The direction seems relevant only for NumberInput, perhaps we'd rather document any "extra" properties on a per component basis.

Makes sense - Moved explanation of such extra properties to <NumberInput>.

vpicone · 2019-06-18T01:55:14Z

packages/react/src/internal/FeatureFlags.js

@@ -17,4 +17,20 @@
 * const MyComponent = props => (<div {...props}>{aFeatureFlag ? 'foo' : 'bar'}</div>);
 */

-/* Currently no feature flag is in use, but keeping this file as a placeholder */
+/**
+ * Uses `value` prop to turn several components to controlled mode, notablly `<NumberInput>`.


With this flag, certain components will be created in either a controlled or controlled mode based on the existence of a value prop. The following components will have the significance of their props slightly altered as outlined below.

Components: <NumberInput>

value → when provided, enables controlled mode. For the rest of the component's lifecycle, it will be controlled by this prop as it's single source of truth.
defaultValue → Optional starting value, used for for uncontrolled mode only (no value prop). The value prop takes precedence over defaultValue.
onChange → optional event handler. However, if value is provided and a handler is not, we'll throw a warning indicating the component is now read-only
readOnly → silences the above warning, acknowledging the read-only state of the component

Thanks @vpicone - Put it in the code.

packages/react/src/internal/FeatureFlags.js

Co-Authored-By: Vince Picone <vpicone@gmail.com>

vpicone

Thanks for all your work/discussion on this. Let me know if you want me to help adding the logic to other components!

asudoh · 2019-06-18T14:50:00Z

Thanks @vpicone for your patience in your review and your offer! It'd be great if you can coordinate with @emyarod for the work. One thought I have is that the feature flag is not needed for components that are not broken by the new mode (e.g. <TextInput>).

emyarod

looks good to me! thanks @asudoh and @vpicone for laying the groundwork for supporting controlled/uncontrolled form inputs based on value prop

refs: carbon-design-system/carbon#3028

asudoh added 2 commits June 12, 2019 17:34

chore(NumberInput): reduce diff

0226d06

asudoh added the status: ready for review 👀 label Jun 12, 2019

asudoh requested a review from vpicone June 12, 2019 08:48

asudoh mentioned this pull request Jun 12, 2019

feat(NumberInput): controlled upon onChange prop #2730

Closed

chore(FeatureFlags): fix stale feature flag name

6747208

emyarod mentioned this pull request Jun 13, 2019

refactor(PasswordInput): use stateless pw input #2655

Closed

vpicone suggested changes Jun 14, 2019

View reviewed changes

asudoh added 2 commits June 17, 2019 14:36

chore(NumberInput): check controlled state in a method

d0388d9

chore(NumberInput): better documentation and file name

1b9c94e

vpicone reviewed Jun 17, 2019

View reviewed changes

packages/react/src/internal/FeatureFlags.js Outdated Show resolved Hide resolved

vpicone reviewed Jun 17, 2019

View reviewed changes

chore(NumberInput): stuck controlled mode to creation-time one

5e4f7f6

asudoh force-pushed the use-controlled-state-with-value branch from 8d7923e to 5e4f7f6 Compare June 17, 2019 23:42

asudoh commented Jun 17, 2019

View reviewed changes

packages/react/src/components/NumberInput/NumberInput.js Show resolved Hide resolved

Merge branch 'master' into use-controlled-state-with-value

6c43a97

vpicone reviewed Jun 18, 2019

View reviewed changes

packages/react/src/internal/FeatureFlags.js Outdated Show resolved Hide resolved

vpicone reviewed Jun 18, 2019

View reviewed changes

packages/react/src/internal/FeatureFlags.js Outdated Show resolved Hide resolved

vpicone suggested changes Jun 18, 2019

View reviewed changes

asudoh and others added 2 commits June 18, 2019 12:17

Update packages/react/src/internal/FeatureFlags.js

b57ba5a

Co-Authored-By: Vince Picone <vpicone@gmail.com>

Update packages/react/src/internal/FeatureFlags.js

1282a72

Co-Authored-By: Vince Picone <vpicone@gmail.com>

asudoh and others added 5 commits June 18, 2019 12:17

Update packages/react/src/internal/FeatureFlags.js

cf3afdd

Co-Authored-By: Vince Picone <vpicone@gmail.com>

Update packages/react/src/internal/FeatureFlags.js

d77156c

Co-Authored-By: Vince Picone <vpicone@gmail.com>

Update packages/react/src/internal/FeatureFlags.js

fe24357

Co-Authored-By: Vince Picone <vpicone@gmail.com>

docs(FeatureFlags): more comments

72a7a9b

chore(NumberInput): remove redundant instance property

f785c96

vpicone approved these changes Jun 18, 2019

View reviewed changes

asudoh added the status: one more review 👀 label Jun 18, 2019

emyarod approved these changes Jun 19, 2019

View reviewed changes

Merge branch 'master' into use-controlled-state-with-value

4a48fc2

asudoh merged commit 64c5953 into carbon-design-system:master Jun 19, 2019

asudoh deleted the use-controlled-state-with-value branch June 19, 2019 00:29

emyarod added a commit to emyarod/carbon-components-react that referenced this pull request Aug 29, 2019

chore: use controlled state with value

ebcc991

refs: carbon-design-system/carbon#3028

emyarod mentioned this pull request Aug 29, 2019

feat(Tooltip): support controlled and uncontrolled modes carbon-design-system/carbon-components-react#2411

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use controlled state with value #3028

Use controlled state with value #3028

asudoh commented Jun 12, 2019

netlify bot commented Jun 12, 2019 •

edited

Loading

netlify bot commented Jun 12, 2019 •

edited

Loading

netlify bot commented Jun 12, 2019 •

edited

Loading

joshblack commented Jun 12, 2019

asudoh commented Jun 13, 2019

vpicone Jun 14, 2019

vpicone Jun 14, 2019

vpicone Jun 14, 2019

vpicone Jun 17, 2019

vpicone left a comment •

edited

Loading

vpicone commented Jun 17, 2019

asudoh left a comment •

edited

Loading

vpicone Jun 18, 2019

asudoh Jun 18, 2019

vpicone Jun 18, 2019

asudoh Jun 18, 2019

vpicone left a comment

asudoh commented Jun 18, 2019

emyarod left a comment

Use controlled state with value #3028

Use controlled state with value #3028

Conversation

asudoh commented Jun 12, 2019

Changelog

Testing / Reviewing

netlify bot commented Jun 12, 2019 • edited Loading

netlify bot commented Jun 12, 2019 • edited Loading

netlify bot commented Jun 12, 2019 • edited Loading

joshblack commented Jun 12, 2019

asudoh commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpicone left a comment • edited Loading

Choose a reason for hiding this comment

vpicone commented Jun 17, 2019

asudoh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpicone left a comment

Choose a reason for hiding this comment

asudoh commented Jun 18, 2019

emyarod left a comment

Choose a reason for hiding this comment

netlify bot commented Jun 12, 2019 •

edited

Loading

netlify bot commented Jun 12, 2019 •

edited

Loading

netlify bot commented Jun 12, 2019 •

edited

Loading

vpicone left a comment •

edited

Loading

asudoh left a comment •

edited

Loading