rfc: "No action" / default property value standardization

[andrefcdias]

New Behavior

This RFC aims to standardize what values we use in our components for cases where a prop value results in no action, i.e. defaults that apply no styles.

Currently, the approach we follow for all components is to use a string value like 'off', 'none' or 'default' for default values of a prop.

PREVIEW 🔍

Related Issue(s)

#23254

[fabricteam]

📊 Bundle size report

_{🤖 This report was generated against ed3ff0780a3bd1ab263a5cd88861b6a353c3dd30}

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 702650b:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

[size-auditor]

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: ed3ff0780a3bd1ab263a5cd88861b6a353c3dd30 (build)

[bsunderhus]

Can you provide an example of a current component that needs this kind of modification right now @andrefcdias? I'm still not 100% sure I'm following your modifications request.

The way I see the value returned from useComponent_unstable state hook should return a well-defined single source of truth state, and having possible undefined values will disassociate logic from state hook into other hooks (in this case useComponentStyle_unstable hook)

If the state value should include the value undefined then maybe using optional parameters is not ideal, perhaps we should be explicitly adding undefined as a type. That would mean that although Required is being used, undefined is still a valid value.

interface OptionalType {
  optional?: string
}
interface NonOptionalType {
  optional: string | undefined
}

const t: Required<OptionalType> = {optional: undefined} // error 🚨
const t2: Required<NonOptionalType> = {optional: undefined} // fine ✅

[andrefcdias]

Can you provide an example of a current component that needs this kind of modification right now @andrefcdias? I'm still not 100% sure I'm following your modifications request.

The way I see the value returned from useComponent_unstable state hook should return a well-defined single source of truth state, and having possible undefined values will disassociate logic from state hook into other hooks (in this case useComponentStyle_unstable hook)

If the state value should include the value undefined then maybe using optional parameters is not ideal, perhaps we should be explicitly adding undefined as a type. That would mean that although Required is being used, undefined is still a valid value.

interface OptionalType {
  optional?: string
}
interface NonOptionalType {
  optional: string | undefined
}

const t: Required<OptionalType> = {optional: undefined} // error 🚨
const t2: Required<NonOptionalType> = {optional: undefined} // fine ✅

This RFC originated from addressing this issue #22855 (PR: #23254). I noted that there wasn't a standard across our components for what the default value should be for when we do nothing with said value. While in Card I'm using 'off' for focusMode, in Text using 'off' would not work as it doesn't make sense for something like fonts (font="off"). Other components, for example Avatar's active prop, use the value "unset", even though this isn't applied the CSS value of "unset" or anything.

It's a fact that there's no standard default value and that quite possibly there's no string value to rule them all, by fitting all use cases. As such, JS standards seem like the appropriate approach to unify behavior and improve DX by avoiding unnecessary documentation reading.

I think that your solution is good, @bsunderhus, as it addresses the problem of standardizing what value represents a no action scenario. However, that comes at the cost of forcing users to initialize a state with all props with the value undefined.

interface OptionalType {
  optional1?: string
  optional2?: string
  optional3?: string
  optional4?: string
  optional5?: string
}
interface NonOptionalType {
  optional1: string | undefined
  optional2: string | undefined
  optional3: string | undefined
  optional4: string | undefined
  optional5: string | undefined
}

const t: OptionalType = { // fine ✅
  optional3: "cool-value"
} 
const t2: NonOptionalType = { // error 🚨
  optional3: "cool-value"
}
const t3: WorkingNonOptionalType = { // fine ✅
  optional1: undefined,
  optional2: undefined,
  optional3: "cool-value",
  optional4: undefined,
  optional5: undefined
}

[andrefcdias]

I don't think we should make this change.

The main motivation from the RFC seems to be to make the useFooStyles_unstable hooks easier to use on their own, by not requiring default values to be given. However, they're not meant to be used on their own. They should be given a state object that was initialized by the useFoo_unstable hook, which should be the single source of truth for the default values.

If there are other motivations, could you please add them to the RFC?

My main arguments to keep the status-quo are:

• For all public APIs (aka props), we are already using undefined to mean "use the default value". And the default value is included in the documentation.

• We haven't settled on the hooks re-composition pattern yet. In some proposals, there is no publicly available useFooStyles-type hook (just a single component hook that returns state, including all styling applied). I don't think we should make changes just for the style hooks at this point, if they might never be made public anyways.

• The default values are all provided explicitly by the useFoo hooks. This gives a single place that defines the default behavior.

  • With the proposal, all other parts of the code need to have their own interpretation of what undefined means. It also makes that code more complex by adding additional checks for undefined, or inventing a '_default' name to use locally.

• Using Required for state props helps catch coding mistakes. E.g. if the dev forgot to include the prop in the state object, or didn't provide a default value when they should have. Sure, we can catch that kind of thing through testing, but IMO it's always better to catch mistakes at compile time if possible.

@behowell

This RFC aims to achieve a standardization of the default values as, as stated in it, there's a plethora of words used and, if we can achieve a standard, the user doesn't need to check documentation.
Also, as stated by some users, these default values can be misleading, like "unset" in Avatar's active prop which does nothing to the component but is a very important keyword in the CSS world that indicates a reset.
This is why I'm proposing not using a default string value but instead reusing the naturally explicit value of undefined as it is a standard across JavaScript development.
From my point of view, if a non-undefined value is an option, it naturally has an action.
i.e in an appearance prop, undefined resolves to a string value that will apply a default style whilst in a prop like Card's focusMode, undefined resolves to an "off" value that should do something, but does not.
If not leverage undefined then coming up with a standard string value that fits all cases seems to me like the only other option.

Also, the current discussions have been around the consequences of the proposal, which is definitely not a blocker for the main topic, so feel free to propose other ideas on how to address the underlying naming problem of default values to avoid the cons you mentioned.
Please also let me know how I can clarify the RFC to reduce confusion over what it's trying to achieve as it's proving to be a common misconception.

[layershifter]

Can I try to summarise this RFC to two statements? (to ensure that my understanding is correct)

• When a component has a default state/behavior, it SHOULD HAVE a default value.

  Example: Card.appearance has 'filled' | 'filled-alternative' | 'outline' | 'subtle' and is filled by default

• When a component does not have a default state/behavior, it SHOULD NOT HAVE a default value.

  Anti-example (current state): Text.font has 'base' | 'monospace' | 'numeric' and defaults to base, where base does not apply any styles
  Example (proposal): Text.font has 'monospace' | 'numeric' and has no defaults (i.e. undefined)

---

@andrefcdias Is it accurate? If so I think that it's a good change that we need to make.

Where I agree with @behowell that we may not need changes to use*Styles_unstable hooks as they are unstable ¯_(ツ)_/¯

This RFC proposes that we leverage the standard JavaScript default, undefined, for attributes instead of a string. For this, we should stop using Required for all props in the State and decide case by case if something should / should not be optional.

Another approach to consider is something like RequiredAllowingUndefined. In this case we will still need to enumerate all properties on state, but undefined will be allowed:

const state3: RequiredAllowingUndefined<Props> = { font: undefined }

However I don't see obvious benefits in this approach as it will only increase bundle size.

[andrefcdias]

Can I try to summarise this RFC to two statements? (to ensure that my understanding is correct)

• When a component has a default state/behavior, it SHOULD HAVE a default value.
  Example: Card.appearance has 'filled' | 'filled-alternative' | 'outline' | 'subtle' and is filled by default
• When a component does not have a default state/behavior, it SHOULD NOT HAVE a default value.
  Anti-example (current state): Text.font has 'base' | 'monospace' | 'numeric' and defaults to base, where base does not apply any styles
  Example (proposal): Text.font has 'monospace' | 'numeric' and has no defaults (i.e. undefined)

@andrefcdias André Dias (he/him) FTE Is it accurate? If so I think that it's a good change that we need to make.

Where I agree with @behowell Ben Howell FTE that we may not need changes to use*Styles_unstable hooks as they are unstable ¯_(ツ)_/¯

This RFC proposes that we leverage the standard JavaScript default, undefined, for attributes instead of a string. For this, we should stop using Required for all props in the State and decide case by case if something should / should not be optional.

Another approach to consider is something like RequiredAllowingUndefined. In this case we will still need to enumerate all properties on state, but undefined will be allowed:

const state3: RequiredAllowingUndefined<Props> = { font: undefined }

However I don't see obvious benefits in this approach as it will only increase bundle size.

That is a great summary to it @layershifter, I'll be adding it to the RFC as it's much better explained and to the point. Thanks!

Also, I think @bsunderhus's proposal is probably the way to go as standardizing is the main concern of the RFC. Thoughts?

[behowell]

Ok, so as I understand now, the main thing to decide here is about the public API: whether we include a named value for a prop's default, or only have undefined. Does that sound right? The other parts about the implementation of the State type and useStyles hooks seem like secondary decisions.

I think this boils down to a stylistic choice. And with all stylistic choices, both options have valid reasons for and against, and can be a source of endless debate 🙂.

I still generally think it's better to leave things as they are (more reasoning below), but I understand if you disagree. I'm going to be on vacation for the next two weeks, so I won't be able to respond during that time. I'll remove my review from the PR to not block further progress.

---

In general, I think when a prop has a union value, the API is clearer if you have names for all possible variants including the default one. That way it's more straightforward to document what happens for each variant: the documentation lists all of the possible values, and then says which one is the default. I would usually expect that undefined would map to one of the available values, rather than itself being a unique variant that's different from the other values... but that's where the stylistic debate comes in.

We could review the "off"/"none"/etc. type values and try to standardize them, but I don't think that full alignment across all props is necessary, since they don't all mean the same thing anyways. For example, something not being focusable focusMode="off" is different from something using the default font family font="base". So there's no reason that they should have the same name.

I'm not entirely opposed to using undefined for some props. But I'm not sure that we should try to make a blanket policy about using undefined for the default value. Especially when the criteria for when to do it are a little hard to nail down... For example, regarding @layershifter's point here:

• When a component does not have a default state/behavior, it SHOULD NOT HAVE a default value.
  Anti-example (current state): Text.font has 'base' | 'monospace' | 'numeric' and defaults to base, where base does not apply any styles
  Example (proposal): Text.font has 'monospace' | 'numeric' and has no defaults (i.e. undefined)

This feels like the criteria is being based on an implementation detail. For what it's worth, I'd say that font="base" does in fact apply styles. The component author chose to put them on the root class, and let the other font values override that, rather than having a separate base class. That's very reasonable, but it could have been written like this instead:

  root: {
-    fontFamily: tokens.fontFamilyBase,
    // ...
  },
+  base: { fontFamily: tokens.fontFamilyBase },
  monospace: { fontFamily: tokens.fontFamilyMonospace },
  numeric: { fontFamily: tokens.fontFamilyNumeric },

  state.root.className = mergeClasses(
    styles.root,
+    state.font === 'base' && styles.base,
    state.font === 'monospace' && styles.monospace,
    state.font === 'numeric' && styles.numeric,
    // ...
  );

That's not to say that's a preferable way to do it, but just to illustrate that it is an implementation detail whether a given value applies styles or not. And implementation details shouldn't drive the API design.

On vacation

[andrefcdias]

Ok, so as I understand now, the main thing to decide here is about the public API: whether we include a named value for a prop's default, or only have undefined. Does that sound right? The other parts about the implementation of the State type and useStyles hooks seem like secondary decisions.

I think this boils down to a stylistic choice. And with all stylistic choices, both options have valid reasons for and against, and can be a source of endless debate 🙂.

I still generally think it's better to leave things as they are (more reasoning below), but I understand if you disagree. I'm going to be on vacation for the next two weeks, so I won't be able to respond during that time. I'll remove my review from the PR to not block further progress.

In general, I think when a prop has a union value, the API is clearer if you have names for all possible variants including the default one. That way it's more straightforward to document what happens for each variant: the documentation lists all of the possible values, and then says which one is the default. I would usually expect that undefined would map to one of the available values, rather than itself being a unique variant that's different from the other values... but that's where the stylistic debate comes in.

We could review the "off"/"none"/etc. type values and try to standardize them, but I don't think that full alignment across all props is necessary, since they don't all mean the same thing anyways. For example, something not being focusable focusMode="off" is different from something using the default font family font="base". So there's no reason that they should have the same name.

I'm not entirely opposed to using undefined for some props. But I'm not sure that we should try to make a blanket policy about using undefined for the default value. Especially when the criteria for when to do it are a little hard to nail down... For example, regarding @layershifter Oleksandr Fediashov aka shift FTE's point here:

• When a component does not have a default state/behavior, it SHOULD NOT HAVE a default value.
  Anti-example (current state): Text.font has 'base' | 'monospace' | 'numeric' and defaults to base, where base does not apply any styles
  Example (proposal): Text.font has 'monospace' | 'numeric' and has no defaults (i.e. undefined)

This feels like the criteria is being based on an implementation detail. For what it's worth, I'd say that font="base" does in fact apply styles. The component author chose to put them on the root class, and let the other font values override that, rather than having a separate base class. That's very reasonable, but it could have been written like this instead:

  root: {
-    fontFamily: tokens.fontFamilyBase,
    // ...
  },
+  base: { fontFamily: tokens.fontFamilyBase },
  monospace: { fontFamily: tokens.fontFamilyMonospace },
  numeric: { fontFamily: tokens.fontFamilyNumeric },

  state.root.className = mergeClasses(
    styles.root,
+    state.font === 'base' && styles.base,
    state.font === 'monospace' && styles.monospace,
    state.font === 'numeric' && styles.numeric,
    // ...
  );

That's not to say that's a preferable way to do it, but just to illustrate that it is an implementation detail whether a given value applies styles or not. And implementation details shouldn't drive the API design.

I feel like the point is being missed @behowell. For that specific case you mentioned "base" is currently being applied by default, but that's not the intended behavior. By default font should not apply styles (read up on #22855 if interested) - no root styles or specific styles. This is what we're trying to solve with this RFC, default values that do nothing in any way to the component should not have a default value string value as that is misleading to the user and requires documentation reading. This is aggravated by the fact that component props use one string value and others another, so the user gains no reusable information when reading the docs on one of the components.
The point with going with undefined is that it's a standard, no documentation is required as it's built into JavaScript development what it "does" (in this case, what it doesn't do).

You're correct when saying that criteria will be based on implementation, but not in the specific way you mention. The rule would be:

• Prop's default value does anything?
  • Yes - Apply a default value
    i.e. appearance, positioning, sizing props
  • No - Keep undefined
    i.e. any prop that does nothing by default

This RFC is not attempting to change the behavior of any props whose default value is meaningful, like the default values used in appearance properties.

tl;dr: If your prop default does nothing, even considering root defined styles, it gets no default value.

[layershifter]

That is a great summary to it @layershifter, I'll be adding it to the RFC as it's much better explained and to the point. Thanks!

👍

---

To clarify: I support that change as it has sense, but I think that RFC is too much focused on implementation details in hooks (that are unstable). I would separate these parts and focus on changes to components' props for now as all other changes could be done later.

[andrefcdias]

I've gone ahead and removed the hook changes, leaving only the standardization of the value.

[khmakoto]

I think I'm fine with approving this now that it has removed the implementation details on hooks. Thanks for doing this!

[layershifter]

@andrefcdias FYI: I updated the link to preview.