Improve ergonomics when using `exactOptionalPropertyTypes`

[OliverJAsh]

Bug report

Current behavior

In projects where exactOptionalPropertyTypes is enabled, we frequently encounter errors like this:

import { Menu } from '@base-ui-components/react';
import type { FC, ReactNode } from 'react';
import * as styles from './MenuItem.module.css';

export const MenuItem: FC<{ disabled?: boolean; children: ReactNode }> = ({
  disabled,
  children,
}) => (
  // Type 'undefined' is not assignable to type 'boolean'.
  <Menu.Item disabled={disabled} className={styles.item}>
    {children}
  </Menu.Item>
);

import { Tooltip } from '@base-ui-components/react';
import type { FC, ReactNode } from 'react';

export const MyTooltip: FC<{
  anchor?: HTMLElement;
  children: ReactNode;
}> = ({ anchor, children }) => (
  <Tooltip.Portal>
    {/* Type 'undefined' is not assignable to type 'Element | VirtualElement | RefObject<Element | null> | (() => Element | VirtualElement | null) | null'. */}
    <Tooltip.Positioner anchor={anchor}>
      <Tooltip.Popup>{children}</Tooltip.Popup>
    </Tooltip.Positioner>
  </Tooltip.Portal>
);

It's possible to workaround these errors using spread syntax, but it's very ugly, and we forgo excess property checking.

Expected behavior

The examples above should not error.

Optional properties should also allow explicit undefined, as is the case for props like className on built-in elements such as div.

Reproducible example

https://codesandbox.io/p/sandbox/condescending-glade-ryyx5d

Base UI version

1.0.0-alpha.8

Which browser are you using?

N/A

Which OS are you using?

N/A

Which assistive tech are you using (if applicable)?

N/A

Additional context

N/A

[mj12albert]

I noticed this was being discussed recently in Material UI as well: https://github.com/mui/material-ui/issues/28745

Many other libs don't support this yet:

• Radix: https://github.com/radix-ui/primitives/issues/3535
• Headless UI: https://github.com/tailwindlabs/headlessui/issues/3388

But the only thing we could do on our side is add | undefined to everything?

[mj12albert]

Also: https://github.com/floating-ui/floating-ui/issues/2818

[atomiks]

This feels like something that should be automated so ?: types all get | undefined, unless there are edge cases?

[OliverJAsh]

For reference, this is what the React types do for built-in components like div: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/0691ed151006d619a77d5ad8104d444906a99502/types/react/index.d.ts#L2670

[flaviendelangle]

This feels like something that should be automated so ?: types all get | undefined, unless there are edge cases?

You mean at the TS compiler level? AFAIK the goal of the exactOptionalPropertyTypes flag is to differentiate undefined and properties and not defined properties. The main difference I can see is when using hasOwnProperty (which will be true for undefined).

I don't see a lot of options other than | undefined and potentially a lint rule to help enforcing it (or not supporting this flag of course)

[OliverJAsh]

Edit: this seems to be resolved now.

Another example of this is when using useRender/mergeProps. Copying the example from the docs, we get an error:

'use client';
import * as React from 'react';
import { useRender } from '@base-ui-components/react/use-render';
import { mergeProps } from '@base-ui-components/react/merge-props';
import styles from './index.module.css';

interface TextProps extends useRender.ComponentProps<'p'> {}

function Text(props: TextProps) {
  const { render = <p />, ...otherProps } = props;

  const { renderElement } = useRender({
    render,
    // Type 'undefined' is not assignable to type 'Ref<HTMLParagraphElement>'.
    props: mergeProps<'p'>({ className: styles.Text }, otherProps),
  });

  return renderElement();
}

[atomiks]

@flaviendelangle

You mean at the TS compiler level?

I mean when building the types for the package automatically, this way we don't need to manually specify it everywhere.

[judehunter]

This would be very nice, since it's just a straight upgrade in type safety to both the library and the users. It doesn't affect users who don't use the flag.

Without the flag, when you use ?, you're basically just telling TypeScript to automatically add that undefined into the union, conflating the meaning of the two. With the flag, you need to manually decide which to use.

For this library, it probably makes to just add undefined to every optional property - though there are use cases where you want one and not the other, of course

[brijeshb42]

I tried two solutions for it and both have their pros and cons.

1. Run a post-build script that just adds | undefined to all optional properties. This would only target the built .d.ts files and won't touch the source.

Pros -

1. Allows users of the lib to enable exactOptionalPropertyTypes in their tsconfig.
2. We (Base UI devs) don't have to worry about adding explicit | undefined.

Cons -

1. Adds | undefined to all optional properties since there is no clear heuristics to operate on. It could be narrowed down to just add it to exported types from a file (but will only operate at the file level and not check the imported file). It may give some falce positives in the process.
2. There is no way for us (Base UI devs) to verify the typesafety since we typecheck only the source and not the built types. We don't even use the built files.

---

2. Run a codemod once on our own source code to add | undefined and every subsequent change by a user will have to add this explicitly (or run the codemod again).

Pros -

1. Users can enable exactOptionalPropertyTypes
2. We run our typecheck on the source files.

Cons -

1. Same heuristics issue as first one when running codemod. Can be narrowed down in a similar way.
2. We cannot enable exactOptionalPropertyTypes on our side since we might be using libraries with similar issues that we are trying to solve here (typecheck failed when using types from @floating-ui/*).
3. Extra overhead on us (Base UI devs) to add the | undefined to new optional properties (though could be automated by a script to fail in CI for this specific use-case). I couldn't find a way to lint this with eslint, unless we create our own custom rule.

So we'll have to pick our poison here cc: @mui/infra @mui/base-ui WDYT?

[benface]

@brijeshb42 What about an ESLint plugin with an auto-fix? Then it could be disabled with a eslint-disable comment as needed.

[LukasTy]

I always prefer less magic, so I would opt for control on our side. But it should be CI/script controlled to avoid the need for intervention and avoid cases, where it's not consistent.

[brijeshb42]

Codemod or custom eslint rule with autofix makes sense. I'll see how easy/complex it is to have the rule.

[michaldudak]

I like the ESLint rule the most. It's hard to think of cases where we'd want an optional prop not to accept explitcit undefined, so there should be no cases of rule suppression (but it's good that we can have this option).