[Badge] Typescript Custom Variant does not work

[michael-land]

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Expected Behavior 🤔

Typescript should not throw error

Steps to Reproduce 🕹

https://codesandbox.io/s/ecstatic-jackson-i6z4j?file=/src/App.tsx

Your Environment 🌎

`npx @material-ui/envinfo`

  System:
    OS: macOS 10.15.6
  Binaries:
    Node: 14.7.0 - /var/folders/7w/t3q6v18575b1rncctdm639040000gn/T/fnm-shell-7427853/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 6.14.7 - /var/folders/7w/t3q6v18575b1rncctdm639040000gn/T/fnm-shell-7427853/bin/npm
  Browsers:
    Chrome: 87.0.4280.141
    Edge: Not Found
    Firefox: 84.0.1
    Safari: 14.0
  npmPackages:
    @emotion/react: 11.1.4 => 11.1.4 
    @emotion/styled: 11.0.0 => 11.0.0 
    @material-ui/core: v5.0.0-alpha.22 => 5.0.0-alpha.22 
    @material-ui/icons: v5.0.0-alpha.22 => 5.0.0-alpha.22 
    @material-ui/lab: v5.0.0-alpha.22 => 5.0.0-alpha.22 
    @material-ui/styled-engine:  5.0.0-alpha.22 
    @material-ui/styles:  5.0.0-alpha.22 
    @material-ui/system:  5.0.0-alpha.22 
    @material-ui/types:  5.1.4 
    @material-ui/unstyled:  5.0.0-alpha.22 
    @material-ui/utils:  5.0.0-alpha.22 
    @types/react: 17.0.0 => 17.0.0 
    react: 17.0.1 => 17.0.1 
    react-dom: 17.0.1 => 17.0.1 
    typescript: 4.1.3 => 4.1.3 

[mnajdova]

Thanks for reporting @xiaoyu-tamu The problem was introduced in #23745 when creating the unstyled component. The problem is that the unstyled component defines the variant as a union of strings, not as overridable string union. I believe we need to move the usage of the OverridableStringUnion from the Badge component tot the BadgeUnstyled. Note that after this, the overriding path for the variant will move to the unstyled package :\ This diff will make this work:

diff --git a/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.d.ts b/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.d.ts
index a2b86fba17..a3427eeaea 100644
--- a/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.d.ts
+++ b/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.d.ts
@@ -1,4 +1,5 @@
 import * as React from 'react';
+import { OverridableStringUnion } from '@material-ui/types';
 import { OverridableComponent, OverridableTypeMap, OverrideProps } from '../OverridableComponent';

 export interface BadgeOrigin {
@@ -6,6 +7,9 @@ export interface BadgeOrigin {
   horizontal: 'left' | 'right';
 }

+export interface BadgePropsVariantOverrides {}
+export type BadgeVariantDefaults = Record<'standard' | 'dot', true>;
+
 export interface BadgeUnstyledTypeMap<P = {}, D extends React.ElementType = 'div'> {
   props: P & {
     /**
@@ -102,7 +106,7 @@ export interface BadgeUnstyledTypeMap<P = {}, D extends React.ElementType = 'div
      * The variant to use.
      * @default 'standard'
      */
-    variant?: 'standard' | 'dot';
+    variant?: OverridableStringUnion<BadgeVariantDefaults, BadgePropsVariantOverrides>;
   };
   defaultComponent: D;
 }
diff --git a/packages/material-ui/src/Badge/Badge.d.ts b/packages/material-ui/src/Badge/Badge.d.ts
index d9446e0534..abb2cd79ea 100644
--- a/packages/material-ui/src/Badge/Badge.d.ts
+++ b/packages/material-ui/src/Badge/Badge.d.ts
@@ -1,6 +1,5 @@
 import * as React from 'react';
 import { SxProps } from '@material-ui/system';
-import { OverridableStringUnion } from '@material-ui/types';
 import {
   ExtendBadgeUnstyledTypeMap,
   BadgeUnstyledTypeMap,
@@ -9,9 +8,6 @@ import {
 import { Theme } from '../styles';
 import { OverridableComponent, OverrideProps } from '../OverridableComponent';

-export interface BadgePropsVariantOverrides {}
-export type BadgeVariantDefaults = Record<'standard' | 'dot', true>;
-
 export type BadgeTypeMap<
   D extends React.ElementType = 'span',
   P = {}
@@ -38,11 +34,6 @@ export type BadgeTypeMap<
      * The system prop that allows defining system overrides as well as additional CSS styles.
      */
     sx?: SxProps<Theme>;
-    /**
-     * The variant to use.
-     * @default 'standard'
-     */
-    variant?: OverridableStringUnion<BadgeVariantDefaults, BadgePropsVariantOverrides>;
   };
   defaultComponent: D;
 }>;

I don't like that we need to move the variants logic to the unstyled component, but for this component, the variant affects the rendered tree of the component itself. Maybe we should try to refactor and move this logic to the styled version 🤷 @oliviertassinari @eps1lon what do you think?

[oliviertassinari]

@mnajdova Did you consider this solution?

diff --git a/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.d.ts b/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.d.ts
index a2b86fba17..48e2b2d5c1 100644
--- a/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.d.ts
+++ b/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.d.ts
@@ -102,7 +102,7 @@ export interface BadgeUnstyledTypeMap<P = {}, D extends React.ElementType = 'div
      * The variant to use.
      * @default 'standard'
      */
-    variant?: 'standard' | 'dot';
+    variant?: string;
   };
   defaultComponent: D;
 }
diff --git a/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.js b/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.js
index 585096889c..3f27c36d8c 100644
--- a/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.js
+++ b/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.js
@@ -206,10 +206,7 @@ BadgeUnstyled.propTypes = {
    * The variant to use.
    * @default 'standard'
    */
-  variant: PropTypes /* @typescript-to-proptypes-ignore */.oneOfType([
-    PropTypes.oneOf(['dot', 'standard']),
-    PropTypes.string,
-  ]),
+  variant: PropTypes.string,
 };

 export default BadgeUnstyled;

On a broader note:

1. In [core] Support public paths in module augmentation  #24267, Sebastian has been pushing so we only augment the modules of public paths. It's already supported but required to update the documentation. When applied to the reproduction of @xiaoyu-tamu: https://codesandbox.io/s/competent-murdock-rf698.
2. Regarding how we will scale the solution, and e.g [theme] Allow custom color variants #13875. What are your thoughts on this pattern?

diff --git a/packages/material-ui-types/index.d.ts b/packages/material-ui-types/index.d.ts
index 378e37492a..93de7d3a0c 100644
--- a/packages/material-ui-types/index.d.ts
+++ b/packages/material-ui-types/index.d.ts
@@ -47,7 +47,7 @@ export type Omit<T, K extends keyof any> = T extends any ? Pick<T, Exclude<keyof
 *
 * @internal
 */
-export type OverridableStringUnion<T, U = {}> = GenerateStringUnion<Overwrite<T, U>>;
+export type OverridableStringUnion<T, U = {}> = GenerateStringUnion<Overwrite<Record<T, true>, U>>;

/**
 * Like `T & U`, but using the value types from `U` where their properties overlap.
diff --git a/packages/material-ui/src/Button/Button.d.ts b/packages/material-ui/src/Button/Button.d.ts
index 93b55007f7..36180c5c2f 100644
--- a/packages/material-ui/src/Button/Button.d.ts
+++ b/packages/material-ui/src/Button/Button.d.ts
@@ -4,8 +4,8 @@ import { Theme } from '../styles';
import { ExtendButtonBase, ExtendButtonBaseTypeMap } from '../ButtonBase';
import { OverrideProps, OverridableComponent, OverridableTypeMap } from '../OverridableComponent';

-export interface ButtonPropsVariantOverrides {}
-export type ButtonVariantDefaults = Record<'text' | 'outlined' | 'contained', true>;
+export interface ButtonPropsOverridesVariant {}
+export interface ButtonPropsOverridesColor {}

export type ButtonTypeMap<
  P = {},
@@ -97,7 +97,7 @@ export type ButtonTypeMap<
     * The color of the component. It supports those theme colors that make sense for this component.
     * @default 'primary'
     */
-    color?: 'inherit' | 'primary' | 'secondary';
+    color?: OverridableStringUnion<'inherit' | 'primary' | 'secondary', ButtonPropsOverridesColor>;
    /**
     * If `true`, the component is disabled.
     * @default false
@@ -145,7 +145,7 @@ export type ButtonTypeMap<
     * The variant to use.
     * @default 'text'
     */
-    variant?: OverridableStringUnion<ButtonVariantDefaults, ButtonPropsVariantOverrides>;
+    variant?: OverridableStringUnion<'text' | 'outlined' | 'contained', ButtonPropsOverridesVariant>;
  };
  defaultComponent: D;
}>;

[mnajdova]

@oliviertassinari right I forgot that I shortly scanned #24267, falling a bit behind after the vacation 😄 Yeah, it makes sense to just use string in the unstyled, if we need to support and refactor the overrides as proposed in your comment 👍

[eps1lon]

I don't like that we need to move the variants logic to the unstyled component, but for this component, the variant affects the rendered tree of the component itself. Maybe we should try to refactor and move this logic to the styled version

Seems to me we haven't gotten to the right abstraction if logic specific to the styled component is part of the unstyled component. I agree that the logic specific to the styled version should be part of the styled implementation. The unstyled component shouldn't know about different variants. In the end, one might want to implement a Badge without variants.

[mnajdova]

Seems to me we haven't gotten to the right abstraction if logic specific to the styled component is part of the unstyled component. I agree that the logic specific to the styled version should be part of the styled implementation. The unstyled component shouldn't know about different variants. In the end, one might want to implement a Badge without variants.

I second this, that is why I mentioned it. For the Badge component, the dot vs standard variant affects the rendered tree of the component itself (whether it will show the content or not for example) - it is not strictly a styling matter. On the other hand, having Badge component without this behavior, seems a bit odd, even unstyled Badge, as this seems to be what most of the badge components on the web behave like, that is why I am not sure whether we should strip things like this form the unstyled version. Maybe the problem is that this is part of the variant's logic, if it was any other prop, like a direct dot boolean prop, I wouldn't have doubts and would leave it in the unstyled component, as it somehow affects the business logic of the component.