RFC: standardize onChange arguments

[dzearing]

Overview

We should provide guidance on how onChange events are exposed across components.

Today we have a discrepancy between v0 and v7 components:

// v7: newValue is the new value of the component's main data, depending on the component type
onChange: (ev: React.FormEvent, newValue:  ___ )

// v0: data is props, with the new value mixed in on top
onChange: (ev: React.FormEvent, data: TProps)

Use cases for onChange

1. The primary data has changed, and I need the new value to support controlled component cases or to forward the data to a state management system.

2. When the data changes I not only need the new value, but I need the original props of the component, so that I can differentiate which instance fired the change event to a general handler.

There are a few types of components which might fire an onChange event:

• Single literal value components: Input and Toggle
• Single-object value components: RadioGroup, Select
• Multi-object selection components: multiselect Dropdown, DetailsList, Tree

Prior considerations

Semantic UI's thread for onChange which provided some background on thinking for Option B below: https://github.com/Semantic-Org/Semantic-UI-React/issues/623#issuecomment-261018287

Material UI follows a similar approach to v7 code: the second argument represents the new value being changed. See Select API as an example of this. However the components aren't consistent - See Input API or Switch API where neither have a second argument.

Proposals

Option A (recommended):

• Standardize on a data object where we provide the following minimum props:

Prop	Description
value	new value for the data of the component
props	the original props given to the parent component

Example:

interface ChangeData<TValue, TProps> {
  value: TValue;
  props: TProps;
}

const onChange = (ev: FormEvent, data: ChangeData<string, InputProps>) => { 
  // I can access the new value.
  console.log(`The new value is ${data.value}`);

  // I can still access the props of the parent component.
  console.log(`The input (#${data.props.id}) user's passed in props are ${JSON.stringify(data.props)`);

 // I can even access additional metadata specific to the change if needed.
 const { id } = rest;
};

Pros

👍 value is always predictable in the value prop 👍 Parent component's current props are accessible and don't overlap other things in the data object 👍 Future proof; data object is extendable without ever accidentally overriding a potentially needed prop value. 👍 Type safety is simple.

Cons

👎 data.props.value and data.props.defaultValue are accessible, leaving multiple ways to see value which might be confusing as they'll represent the current props rather than the new value. (But it should be obvious that these are user inputs and not the new value. (Could consider calling new value newValue to be clear, but that seems a bit unpredictable.)

Option B: stick with v0 approach - data is { ...props, value }

Example:

const onChange = (ev: FormEvent, props) => { 

  // I can access the new value.
  console.log(`The new value is ${props.value}`);

  // I can still access the props of the parent component.
  console.log(`The input (#${props.id}) user's passed in props are ${JSON.stringify(props)`);

  // I can access additional metadata specific to the change if needed, but it may overwrite 
  // original props values because of the lack of namepacing.
  const { index } = props;

};

Pros

👍 Can access parent props mostly (unless it was overridden by some selection data.) 👍 One copy of value (defaultValue could still be there.)

Cons

👎 Mixing additional meta data about the change (such as index or virtualized selection information) runs the risk of overlapping on the parent's props. (e.g. index prop of the parent vs index of the new selected item) 👎 Slightly less efficient (copy all props over vs just set the props value)

Option C: stick with v7 approach - second argument is the new value

Example:

const onChange = (ev: FormEvent, value) => { 

  // I can access the new value.
  console.log(`The new value is ${value}`);

  // I can not access the props of the originating component; `ev.target` may handle some of this:
  console.log(`The input (#${ev.target.id}) change its value to ${value}.`);

 // I can not access additional metadata without baking it into `value`, which may require a breaking change.
 const { index } = value;

};

TextField uses string for the newValue type. Toggle uses boolean for the newValue type. ChoiceGroup uses IChoiceGroupOption as newValue type.

Pros

👍 Main use case of getting new value is straightforward 👍 No breaking changes with v7, less churn for customers

Cons

👎 Can't access props of parent component from callback, so to do things like sharing one callback implementation which maintains a form hash table requires accessing ev.target.id to know which component changed.

👎 Adding additional information to the data like index or key is a breaking change.

[layershifter]

@dzearing can you please add to each proposal an example for signature like this:

onChange: (ev: React.FormEvent, data: TProps)

Currently, I don't understand difference between options.

[dzearing]

@layershifter done;

A: move props into a member of data B: spread props into data C: data is just the new value

A is the easiest to extend as needed in the future, similar to how HTMLEvent is architected, where the event arg data contains not only the element but a much of metadata about the origin and context of the change. We likely will need this sort of context in the future as well. For example, a RadioGroup value may be the RadioGroupItem that was selected, but the index of the selection may also be required, so that would be additional metadata on the data object we could add, without worrying how it affects props mixed in.

[msft-fluent-ui-bot]

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.