React 18 Enhancements: Ensure default props are initialized with serially-stable values

[dcwarwick]

Package

carbon-components-react

Package version

10.53.0

Description

I notice that several places where defaultProps were being used before, this has been removed and instead the default prop values are being supplied as ES6 destructuring initialisers. E.g. Button, TextInput. This is fine, of course, as this is now a preferred way of handling default prop values.

However, there is a tricky anti-pattern to watch out for. The destructuring initialisers will run EVERY TIME the render function is called, so whenever a prop is NOT supplied the default value will be inserted on EVERY CALL to the render function. This is ok if the default value is primitive, or any other serially stable value: when React checks what has changed it will consider these props to have the same values and not trigger any hooks that depend on them, including rendering. However, if the default value is something that is not serially stable, the prop will appear to React to have a new value every time the function is called, and so affected hooks and rendering will happen EVERY TIME. This is usually not harmful except that it is not optimal. Optimization is one of the motivations for switching the way that default prop values are supplied.

Some examples of values that are not serially stable:

• []
• () => {}

For example, TextInput now has the following two default prop values:

    onChange = () => {},
    onClick = () => {},

The onChange and onClick props, if not supplied, will be initialised to a new arrow function on every call, which React will see as a new prop value on every call, so it’ll re-render the text input on every call.

Also, Button has the following default prop value:

    size = FeatureFlags.enabled('enable-v11-release') ? 'lg' : 'default',

Now, this is serially stable, but it will check the feature flag on every invocation, which is perhaps not ideal either.

When the initialization expression is not serially stable, the initialization needs to be done outside the function parameter list. So, for example, the TextInput could instead do:

const noopFn = () => {};

// ...

  onChange = noopFn,
  onClick = noopFn,

and this will now be fine.

Similarly, Button could do:

const defaultSize = FeatureFlags.enabled('enable-v11-release') ? 'lg' : 'default',

// ...

    size = defaultSize,

and that will ensure the default value is only calculated once.

For @carbon/ibm-products, we have just applied a common pattern in which ALL of the default values are actually set up in an object, and then referenced from the parameter list. This has several benefits: not only does it help ensure we don't slip into the above anti-pattern by oversight, but it also gathers the default values together in a nice neat way, making it easy to check, for example, that none of the defaulted parameters is also marked required, and providing a place to comment unusual values, etc. This approach could be applied, for example, to TextInput by doing something like:

const defaults = {
  disabled: false,
  inline: false,
  invalid: false,
  light: false,
  onChange = () => {},
  onClick = () => {},
  size: 'md',
  type: 'text',
  warn: false,
};

// ...

const TextInput = React.forwardRef(function TextInput(
  {
    className,
    disabled = defaults.disabled,
    helperText,
    hideLabel,
    id,
    inline = defaults.inline,
    invalid = defaults.invalid,
    invalidText,
    labelText,
    light = defaults.light,
    onChange = defaults.onChange,
    onClick = defaults.onClick,
    placeholder,
    readOnly,
    size = defaults.size,
    type = defaults.type,
    warn = defaults.warn,
    warnText,
    ...rest

You can see how I've applied the above convention in this PR: https://github.com/carbon-design-system/ibm-cloud-cognitive/pull/1654/files. For example, this file Card.js might be a good example to look at: https://github.com/carbon-design-system/ibm-cloud-cognitive/blob/86b8c98c32cd6426378e66c19cf1c3359ae53b07/packages/cloud-cognitive/src/components/Card/Card.js. I don't know if this is the best approach, but it seems to me to have some advantages, so I mention it in case it is of interest. It seems to me that by having a clear place where the default values are placed will help ensure that future updates don't introduce this problem again, as it is not an obvious issue for developers to have in their mind.

In any case, the default values that currently use expressions that are not serially stable should be fixed to initialize outside the parameter list, and preferably some systematic structure to help ensure this is consistently maintained going forward would be put in place.

I hope this helps.

Code of Conduct

• I agree to follow this project's Code of Conduct
• I checked the current issues for duplicate problems

[dcwarwick]

@sstrubberg @joshblack FYI
And the following thread on Slack is the stream-of-consciousness where I realised this could be an issue: https://ibm-casdesign.slack.com/archives/C013ZTX0N6B/p1645033906734629

[falsepopsky]

I'm not sure of the current status of this proposal, but in addition to what @dcwarwick said earlier (I totally agree), it's worth noting that defaultProps for function components will be deprecated in the next version of React.

reactjs/rfcs#107

[tay1orjones]

@falsepopsky Yeah, great point. We need to refactor off of defaultProps and use default parameters instead. We added this issue as part of our React 18 umbrella issue #11308

On the main topic of "serially stable" values - this matches the recommendation from the React team a few years back: facebook/react#16905 (comment)

I'm not sure if theres any new approaches out there for us to consider in addition to this.

[tay1orjones]

The best way to approach this is probably going to be assigning multiple team members during a single sprint to tackle this in a mob programming session alongside #13424

[icyJoseph]

And now it seems like Function Components stop supporting defaultProps altogether.

• Enable warning for defaultProps on function components for everyone facebook/react#25699