[Slider][material] Fix type dependency on @types/prop-types

[Methuselah96]

• I have followed (at least) the PR section of the contributing guide.

There is a type error when using a strict package manager because SliderValueLabel.d.ts imports from @types/prop-types, but @mui/material has no dependency on @types/prop-types:

ERROR in ./node_modules/@mui/material/Slider/SliderValueLabel.d.ts:2:23
TS7016: Could not find a declaration file for module 'prop-types'. 'C:/Users/nbier/Documents/ParagonCore/.yarn/cache/prop-types-npm-15.8.1-17c71ee7ee-c056d3f1c0.zip/node_modules/prop-types/index.js' implicitly has an 'any' type.    
  If the 'prop-types' package actually exposes this module, consider sending a pull request to amend 'https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/prop-types'
    1 | import * as React from 'react';
  > 2 | import PropTypes from 'prop-types';
      |                       ^^^^^^^^^^^^
    3 | import { SliderValueLabelProps } from './SliderValueLabel.types';
    4 | /**
    5 |  * @ignore - internal component.

This PR fixes this error by adding a dependency on @types/prop-types.

[mui-bot]

Netlify deploy preview

https://deploy-preview-37853--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 2c88d34

[oliviertassinari]

I don't think that these component types should depend on prop-types in the first place, this looks very strange to me: https://unpkg.com/browse/@mui/material@5.13.7/Slider/SliderValueLabel.d.ts. I also suspect that this slows TypeScript down.

So I would propose we remove the dependency in the first place:

diff --git a/packages/mui-material/src/Slider/SliderValueLabel.tsx b/packages/mui-material/src/Slider/SliderValueLabel.tsx
index d5f1364b87..fef5077057 100644
--- a/packages/mui-material/src/Slider/SliderValueLabel.tsx
+++ b/packages/mui-material/src/Slider/SliderValueLabel.tsx
@@ -49,4 +49,4 @@ SliderValueLabel.propTypes = {
   children: PropTypes.element.isRequired,
   className: PropTypes.string,
   value: PropTypes.node,
-};
+} as any;

Same for Material UI v6 cc @DiegoAndai

diff --git a/packages/mui-material-next/src/Slider/SliderValueLabel.tsx b/packages/mui-material-next/src/Slider/SliderValueLabel.tsx
index d5f1364b87..fef5077057 100644
--- a/packages/mui-material-next/src/Slider/SliderValueLabel.tsx
+++ b/packages/mui-material-next/src/Slider/SliderValueLabel.tsx
@@ -49,4 +49,4 @@ SliderValueLabel.propTypes = {
   children: PropTypes.element.isRequired,
   className: PropTypes.string,
   value: PropTypes.node,
-};
+} as any;

Per

$ grep -inr --include \*.d.ts "import PropTypes from 'prop-types';"
./build/Slider/SliderValueLabel.d.ts:2:import PropTypes from 'prop-types';

It's the only case in @mui/material but this impacts other packages and could regress in the future. So, I would go with

diff --git a/scripts/buildTypes.mjs b/scripts/buildTypes.mjs
index d8d3c1977f..66495ecf56 100644
--- a/scripts/buildTypes.mjs
+++ b/scripts/buildTypes.mjs
@@ -54,6 +54,22 @@ async function main() {

   async function rewriteImportPaths(declarationFile) {
     const code = await fse.readFile(declarationFile, { encoding: 'utf8' });
+    const basename = path.basename(declarationFile);
+
+    if (
+      // Only consider React components
+      basename[0] === basename[0].toUpperCase() &&
+      code.indexOf("import PropTypes from 'prop-types';") !== -1
+    ) {
+      throw new Error(
+        [
+          `${declarationFile} imports from 'prop-types', this is wrong.`,
+          "It's likely missing a cast to any on the propTypes declaration:",
+          'ComponentName.propTypes = { /* prop */ } as any;',
+        ].join('\n'),
+      );
+    }
+
     let fixedCode = code;
     const changes = [];

as well, and fix @mui/material-next (all the other packages seem correct, MUI X packages included, I run the diff there too)

[Methuselah96]

I have updated the PR to do an as any cast instead of adding the dependency.

[Methuselah96]

@oliviertassinari Let me know if there's anything else I can do. I can make the changes to buildTypes.mjs as well, but wasn't sure if you wanted that in this PR.

[DiegoAndai]

Thanks for the contribution @Methuselah96!

Also I think we should implement the changes @oliviertassinari proposed to buildTypes.mjs in this same PR

We don't need to do it for v6 Slider, with the restructure PR: #37644 (just merged), the SliderValueLabel file was removed

[Methuselah96]

@DiegoAndai Sounds good. I've updated the usages of propTypes in mui-material-next to use as any, however there's still one usage that's still referencing prop-types and giving me trouble. The TouchRippleRipple in mui-material-next/Button/TouchRipple is coming through as:

export declare const TouchRippleRipple: import("@emotion/styled").StyledComponent<Pick<Pick<import("./Ripple").RippleProps, keyof import("./Ripple").RippleProps> & Pick<PropTypes.InferProps<any>, string | number | symbol> & Pick<import("./Ripple").RippleProps, never>, keyof import("./Ripple").RippleProps> & import("@mui/system").MUIStyledCommonProps<import("@mui/material").Theme>, {}, {}>;

Any ideas on how to resolve this?

[Methuselah96]

I assume this is because the Ripple component is coming through as:

declare function Ripple(props: RippleProps): React.JSX.Element;
declare namespace Ripple {
    var propTypes: any;
}

and so some type is probably detecting the propTypes property and mapping it to PropTypes.InferProps<any>. I'm having trouble thinking of a good solution.

[Methuselah96]

Alright, this is a little janky, but I changed Ripple.propTypes = {} as any; to (Ripple as any).propTypes = {}; and that fixes it. Hopefully that fix is acceptable.

[Methuselah96]

The failing build is complaining about material-ui/packages/mui-lab/build/DateRangePicker/DateRangePickerView.d.ts, but I am unable to figure out where that file is coming from and it does not fail for me locally. Any help?

[DiegoAndai]

There's something I don't understand, are the Ripple and SliderValue files the only ones causing the issue? We use proptypes for all components, why aren't there other files causing issues?

About the DateRangePickerView CI error, let's see if @oliviertassinari or @michaldudak know something about it 🤔 maybe some component that was moved and shouldn't be there anymore

[Methuselah96]

@DiegoAndai Yes, those are the only components causing issues. It looks like the other components aren't causing issues because either:

1. The component is written in JS and the TS declaration file doesn't include prop-types.
2. The .propTypes property is cast to any so therefore prop-types is not getting imported in the declaration file.
3. The component itself is cast, so the .propTypes property isn't making it into the declaration file.

[oliviertassinari]

Alright, this is a little janky, but I changed Ripple.propTypes = {} as any; to (Ripple as any).propTypes = {}; and that fixes it. Hopefully that fix is acceptable.

@Methuselah96 It would indeed make more sense to me, this .propType property is removed in production https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types/tree/master#example, so it shouldn't be visible in the first place:

diff --git a/packages/typescript-to-proptypes/src/generator.ts b/packages/typescript-to-proptypes/src/generator.ts
index face64ba54..de046a8619 100644
--- a/packages/typescript-to-proptypes/src/generator.ts
+++ b/packages/typescript-to-proptypes/src/generator.ts
@@ -334,8 +334,11 @@ export function generate(component: t.Component, options: GenerateOptions = {}):
   const propTypesMemberTrailingComment = ensureBabelPluginTransformReactRemovePropTypesIntegration
     ? '/* remove-proptypes */'
     : '';
-  const propTypesCasting = disablePropTypesTypeChecking ? ' as any' : '';
   const propTypesBanner = comment !== undefined ? comment : '';

-  return `${component.name}.propTypes ${propTypesMemberTrailingComment}= {\n${propTypesBanner}${generated}\n}${propTypesCasting}`;
+  if (disablePropTypesTypeChecking) {
+    return `(${component.name} as any).propTypes ${propTypesMemberTrailingComment}= {\n${propTypesBanner}${generated}\n}`;
+  }
+
+  return `${component.name}.propTypes ${propTypesMemberTrailingComment}= {\n${propTypesBanner}${generated}\n}`;
 }

However, it would be way too many changes for this PR, let's stick to the current convention to keep the PR small in scope.

The failing build is complaining about material-ui/packages/mui-lab/build/DateRangePicker/DateRangePickerView.d.ts, but I am unable to figure out where that file is coming from and it does not fail for me locally. Any help?

Rebasing on HEAD seems to fix it, unless it's removing the dead restore_cache: that fixed it.

There's something I don't understand, are the Ripple and SliderValue files the only ones causing the issue? We use proptypes for all components, why aren't there other files causing issues?

@DiegoAndai Most of the files for Material UI v5 are in JavaScript, we have been manually authoring the TypeScript definitions (.d.ts) precisely to solve this class of issues we solve with this PR.

Now, the types of the Slider in Material UI v6 look a bit broken:

• React.FC< adds the children prop which many of the slots don't support.
• Props of slots are not type checked, it accepts anything, I wonder if we would want to copy the typeof SliderValueLabelComponent.

[DiegoAndai]

This looks ready to merge to me, let's wait and see if @oliviertassinari has anything else to add

Now, the types of the Slider in Material UI v6 look a bit broken:

• React.FC< adds the children prop which many of the slots don't support.
• Props of slots are not type checked, it accepts anything, I wonder if we would want to copy the typeof SliderValueLabelComponent.

I will review these separately, no need to fix those here, thanks for pointing those out.

[DiegoAndai]

Changes are ready to merge 🚀

@Methuselah96 have you tested the fix after all the changes? is it fixing the original issue you had?

[Methuselah96]

The changes in the built declaration files look correct and should fix the issue. I haven't got the chance to actually try it in my app, but I'm confident it will fix the original issue.