@material-ui/core: parsed: -0.07% 😍, gzip: -0.06% 😍

Details of bundle changes

Generated by 🚫 dangerJS against c006911

Looks good to me. I’d say we should probably use the xxxClassKeys as xxxOveridesKeys and add them only in the theme typings. As we don’t support overrides for the pseudo states, it can be better if we can restrict and distinguish them from the classes prop.

Also depending on the final decision can you please update the comment we have on the migration RFC?

We might want to revisit why we need a separate file. If a file A is only ever used in a file B we might as well merge them to remove indirection.

I like this. cc @mnajdova

And we might want to revisit why we need a separate file.

@eps1lon I have explored removing the sliderUnstyledClasses.js file. I have found that the SliderValueLabelUnstyled needs to access the exported variable, but SliderUnstyled also needs to import SliderValueLabelUnstyled. It creates a cycle dependency. So it seems that the file was introduced by Marija to avoid a cycle dependency.

It seems that the current state of the pull request works because we have the .js and .d.ts files isolate, with no cycle dependency strictly speaking inside each realm (TS vs. JS).

Seems to me we need to rethink the documentation approach anyway. The classes are no longer synced.

What do you mean by the classes are no longer synced?

I see two options.

Option 1

• In the long term, we move the description of the classes to where the classes is declared (sliderUnstyledClasses.d.ts in this example):

 import { OverridableComponent, OverridableTypeMap, OverrideProps } from '../OverridableComponent';
+import { SliderUnstyledClasses } from './sliderUnstyledClasses';
 
 export interface Mark {
   value: number;
@@ -30,40 +31,7 @@ export interface SliderUnstyledTypeMap<P = {}, D extends React.ElementType = 'sp
      * Override or extend the styles applied to the component.
      * @default {}
      */
-    classes?: {
-      /** Class name applied to the root element. */
-      root?: string;
-      /** Class name applied to the root element if `marks` is provided with at least one label. */
-      marked?: string;
-      /** Class name applied to the root element if `orientation="vertical"`. */
-      vertical?: string;
-      /** Pseudo-class applied to the root and thumb element if `disabled={true}`. */
-      disabled?: string;
-      /** Class name applied to the rail element. */
-      rail?: string;
-      /** Class name applied to the track element. */
-      track?: string;
-      /** Class name applied to the track element if `track={false}`. */
-      trackFalse?: string;
-      /** Class name applied to the track element if `track="inverted"`. */
-      trackInverted?: string;
-      /** Class name applied to the thumb element. */
-      thumb?: string;
-      /** Pseudo-class applied to the thumb element if it's active. */
-      active?: string;
-      /** Pseudo-class applied to the thumb element if keyboard focused. */
-      focusVisible?: string;
-      /** Class name applied to the thumb label element. */
-      valueLabel?: string;
-      /** Class name applied to the mark element. */
-      mark?: string;
-      /** Class name applied to the mark element if active (depending on the value). */
-      markActive?: string;
-      /** Class name applied to the mark label element. */
-      markLabel?: string;
-      /** Class name applied to the mark label element if active (depending on the value). */
-      markLabelActive?: string;
-    };
+    classes?: Partial<SliderUnstyledClasses>;
     /**
      * The components used for each slot inside the Slider.
      * Either a string to use a HTML element or a component.
@@ -269,6 +237,6 @@ export type SliderUnstyledProps<
   P = {}
 > = OverrideProps<SliderUnstyledTypeMap<P, D>, D>;
 
-export type SliderUnstyledClassKey = keyof NonNullable<SliderUnstyledTypeMap['props']['classes']>;
+export type SliderUnstyledClassKey = keyof SliderUnstyledClasses;

• In the long term, we update the updateStylesDefinition() function to be compatible with the approach. It should be easy to handle. I think that once we have migrated everything, I could do it in an hour or two.
• In the short term, we duplicate the classes, we copy the content of the main TS file into the classes file.

Option 2

We move forward with the current direction of the pull request. At least, to avoid types duplication.

Any preferences or alternatives in mind?

Regarding removing the xxxClasses files. I don't have preferences. I'm used to how it's now, I can understand why a single file too. Only a few components will require it. Any preferences?

What do you mean by the classes are no longer synced?

packages/material-ui-unstyled/src/SliderUnstyled/sliderUnstyledClasses.d.ts has (removed in this PR) a valueLabelCircle class. This class is not present in packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.d.ts#L33

I have pushed forward with Option 2. It seems to be the simplest way to remove the duplication. I think that Option 1 has potential in the long-term. I can migrate all the other components if we are happy with this approach.

I can do option 1. But probably not before next week. Updating them manually for now is fine.

@eps1lon Ok, it sounds like a plan. I might suggest we defer option 1 a bit, maybe once we have migrated all the components to emotion so that we don't need to maintain two different API docs script generation.

maybe once we have migrated all the components to emotion so that we don't need to maintain two different API docs script generation.

I'm not sure if you're trying to do some gotcha with me saying "maintenance is hard" but there are worlds between maintaining a public API and maintaining an internal solution. Internal code has almost no cost to change. Calling it "maintaining" is just wrong at this point. It seems like there's either already a problem with the new behavior which you're trying to avoid or you just don't want to do it. But maintenance has nothing to do with it.

@eps1lon Ok sounds fair :)