mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.85k stars 32.25k forks source link

[Error] Incorrect Import of PropTypes Causes Compilation Error #43700

Closed morozow closed 1 month ago

morozow commented 1 month ago

Steps to reproduce

Link to live example: local dev as there's no TS completion to build live phase

Steps:

  1. Set up a Material UI project 6.0.2 with TypeScript.
  2. Use a Material UI component that imports PropTypes with import PropTypes from 'prop-types';.
  3. Run the TypeScript compiler.

Current behavior

When using Material UI with TypeScript, I encountered a compilation error due to the incorrect use of import PropTypes from 'prop-types'; in the codebase.

Typescript Compiler error:

node_modules/@mui/utils/chainPropTypes/chainPropTypes.d.ts:1:8 - error TS1192: Module '"path/to/project/node_modules/@types/prop-types/index"' has no default export.

1 import PropTypes from 'prop-types';

Current example of Material UI implementation: https://github.com/mui/material-ui/blob/bb620d85a89651ca4350e8861bbb35776d583726/packages/mui-utils/src/chainPropTypes/chainPropTypes.ts#L1

Since the prop-types library does not have a default export, this import statement is incorrect and should instead use named imports to align with ES6 module standards. https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/prop-types/index.d.ts

Solution: apply import * as PropTypes from 'prop-types';

Expected behavior

The project should compile without errors.

Context

Replace all occurrences of import PropTypes from 'prop-types'; with import * as PropTypes from 'prop-types'; in the entire Material UI codebase.

Additional Context

Your environment

npx @mui/envinfo ``` System: OS: macOS 14.1.1 Binaries: Node: 20.9.0 - /usr/local/bin/node npm: 10.2.4 - /usr/local/bin/npm pnpm: Not Found Browsers: Chrome: 128.0.6613.120 Edge: Not Found Safari: 17.1 npmPackages: @emotion/react: ^11.13.3 => 11.13.3 @emotion/styled: ^11.13.0 => 11.13.0 @mui/core-downloads-tracker: 6.0.2 @mui/envinfo: ^2.0.25 => 2.0.25 @mui/icons-material: ^6.0.2 => 6.0.2 @mui/material: ^6.0.2 => 6.0.2 @mui/private-theming: 6.0.2 @mui/styled-engine: 6.0.2 @mui/styles: ^6.0.2 => 6.0.2 @mui/system: 6.0.2 @mui/types: 7.2.16 @mui/utils: 6.0.2 @mui/x-date-pickers: ^7.16.0 => 7.16.0 @mui/x-internals: 7.16.0 @types/react: ^18.3.5 => 18.3.5 react: ^18.3.1 => 18.3.1 react-dom: ^18.3.1 => 18.3.1 typescript: ^5.6.2 => 5.6.2 ```

Search keywords: error, types, d.ts

morozow commented 1 month ago

Nitpick: If anyone encounters this issue, here is a quick fix for Unix systems using Yarn post-install:

#!/bin/bash
cd ./

echo "Tuning of TypeScript types for React node_modules packages..."

# Find all .ts files in the project directory including node_modules and replace the import statement
find . -type f -name "*.ts" -exec sed -i '' 's/import PropTypes from '"'"'prop-types'"'"';/import * as PropTypes from '"'"'prop-types'"'"';/g' {} +

# Find all .ts files in the project directory including node_modules and replace the import statement
find . -type f -name "*.ts" -exec sed -i'' -e 's#import React[^;]*#import * as React from '"'"'react'"'"'#g' {} +

Smth like this into package.json:

...
"scripts": {
    "postinstall": "bash ./bin/post.install.sh",
}
...
morozow commented 1 month ago

And to add my 2 cents, given that we’re facing this issue as the optimal fix appears to involve the Unix sed command, pointing to a deployment issue with Material UI. And just take a note, Bitbucket and other deployment environments by git lack access to the Unix sed command. Specifically, my process involves locally building a React application that includes Material UI, then pushing this build to a Git repository. Following this, the deployment can be executed to CloudFront using bitbucket-pipelines.yml as one option, or it can also be done locally through a simple bash script. However, using the local script option means missing out on collaborative team challenges. Could we prioritise resolving this issue as soon as possible please?

Here's a sample Bash script for deploying a React application that uses Material UI:

#!/bin/bash
cd ./

echo "Deploying Material UI Application to CloudFront..."

# IMPORTANT: before deploy you MUST/CAN clear all S3 Bucket files then to deploy, by the other case you can meet a conflict or S3 Bucket potential overload

aws s3 sync ./build s3://$CLOUDFRONT_DISTRIBUTION_S3_BUCKET_NAME/
aws cloudfront create-invalidation --distribution-id $CLOUDFRONT_DISTRIBUTION_ID --paths "/*" | grep Id

echo "CloudFront invalidation is going..."
exit 0
morozow commented 1 month ago

I have verified that each package includes "@types/prop-types": "^15.7.12", which indicates that the entire project can transition to the correct PropTypes import syntax for TypeScript compatibility.

Here is the Pull Request addressing this fix: https://github.com/mui/material-ui/pull/43701

Please review it and let me know if there are any additional contributions or changes needed.

oliviertassinari commented 1 month ago

I changed those imports in #23116, maybe it was wrong 🤔

morozow commented 1 month ago

@Janpot I've tailored the solution to directly address the core issue with the tsc in the upcoming PR https://github.com/mui/material-ui/pull/43736.

We can either keep this https://github.com/mui/material-ui/pull/43701 PR open for future reference or close it, depending on your preference. However, we should consider enhancing a codebase with a new PR by incorporating utilities to enable tsc

morozow commented 1 month ago

@oliviertassinari The issue with tsc primarily pertains to *.d.ts into utils files. From what I can tell, your enhancement doesn’t seem to break the TypeScript behaviour

Janpot commented 1 month ago

I changed those imports in #23116, maybe it was wrong 🤔

I think you were correct. I believe @types/prop-types is wrong.

oliviertassinari commented 1 month ago

@Janpot Yeah agree, the problem is not with Material UI.

In https://unpkg.com/browse/prop-types@15.8.1/index.js, I see: module.exports =. So it should be imported like this on Node.js:

const propTypes1 = require('prop-types');

import propTypes2 from 'prop-types';
import * as propTypes3 from 'prop-types';

propTypes1. // valid
propTypes2. // valid
propTypes3.default. // valid

let's fix @types/prop-types instead: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/70539.

morozow commented 1 month ago

@Janpot @oliviertassinari I might be mistaken, but it appears that we should refer to the @types/prop-types package: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/prop-types/index.d.ts to address the concerns raised in my pull requests like https://github.com/mui/material-ui/pull/43736

morozow commented 1 month ago

@oliviertassinari It's impressive how smoothly the previous decision was altered by https://github.com/mui/material-ui/issues/43700#issuecomment-2347374665. Just a quick observation: the updated PR description still seems incorrect after update, as it references a *.ts file instead of the needed @types/prop-types options. I was happy to assist. I look forward to the next release, as the current issue halts all deliveries. Thank you.

Janpot commented 1 month ago

@morozow We appreciate your willingness to contribute, but to reduce the risk of contributions being rejected, it would be best to wait until we've agreed on the appropriate solution. To be fully honest, I still don't understand what the exact problem is. At this point, the best thing you could do is to share a real reproduction (e.g. a codesandbox, or a github repository) that clearly demonstrates what you're trying to do and the error you're running into so that we can take a look. These imports have been there in this form since 2020, without any issues, I hope you understand that we're not going to make hasty changes to to them, especially when it's demonstrated they will be harmful in the future.

It's clear that there is a mismatch between what the prop-types package exports and what the @types/prop-types declares. You may have to adjust your typescript configuration to get it compiled (see allowSyntheticDefaultImports).

morozow commented 1 month ago

@Janpot I've set up a fresh repository at https://github.com/morozow/material-ui-tsc-fix. I started by adding a default tsconfig.json following the minimal configuration guidelines from https://mui.com/material-ui/guides/typescript/#minimum-configuration. Next, I installed Material UI as instructed at https://mui.com/material-ui/getting-started/installation/ by npm. After that, I created a default example using the instructions from https://mui.com/material-ui/guides/typescript/#complications-with-the-component-prop. Upon running tsc, I observe the following:

user@machine material-ui-tsc-fix % tsc
node_modules/@mui/system/borders/index.d.ts:1:10 - error TS2305: Module '"./borders"' has no exported member 'default'.

1 export { default } from './borders';
           ~~~~~~~

node_modules/@mui/system/cssGrid/index.d.ts:1:10 - error TS2305: Module '"./cssGrid"' has no exported member 'default'.

1 export { default } from './cssGrid';
           ~~~~~~~

node_modules/@mui/system/display/index.d.ts:1:10 - error TS2305: Module '"./display"' has no exported member 'default'.

1 export { default } from './display';
           ~~~~~~~

node_modules/@mui/system/flexbox/index.d.ts:1:10 - error TS2305: Module '"./flexbox"' has no exported member 'default'.

1 export { default } from './flexbox';
           ~~~~~~~

node_modules/@mui/system/palette/index.d.ts:1:10 - error TS2305: Module '"./palette"' has no exported member 'default'.

1 export { default } from './palette';
           ~~~~~~~

node_modules/@mui/system/positions/index.d.ts:1:10 - error TS2305: Module '"./positions"' has no exported member 'default'.

1 export { default } from './positions';
           ~~~~~~~

node_modules/@mui/system/shadows/index.d.ts:1:10 - error TS2305: Module '"./shadows"' has no exported member 'default'.

1 export { default } from './shadows';
           ~~~~~~~

node_modules/@mui/system/sizing/index.d.ts:1:10 - error TS2305: Module '"./sizing"' has no exported member 'default'.

1 export { default } from './sizing';
           ~~~~~~~

node_modules/@mui/system/spacing/index.d.ts:1:10 - error TS2305: Module '"./spacing"' has no exported member 'default'.

1 export { default } from './spacing';
           ~~~~~~~

node_modules/@mui/system/style/index.d.ts:1:10 - error TS2305: Module '"./style"' has no exported member 'default'.

1 export { default } from './style';
           ~~~~~~~

node_modules/@mui/system/typography/index.d.ts:1:10 - error TS2305: Module '"./typography"' has no exported member 'default'.

1 export { default } from './typography';
           ~~~~~~~

node_modules/@mui/utils/chainPropTypes/chainPropTypes.d.ts:1:8 - error TS1192: Module '"/path/to/sandbox/material-ui/material-ui-tsc-fix/node_modules/@types/prop-types/index"' has no default export.

1 import PropTypes from 'prop-types';
         ~~~~~~~~~

node_modules/@mui/utils/elementAcceptingRef/elementAcceptingRef.d.ts:1:8 - error TS1192: Module '"/path/to/sandbox/material-ui/material-ui-tsc-fix/node_modules/@types/prop-types/index"' has no default export.

1 import PropTypes from 'prop-types';
         ~~~~~~~~~

node_modules/@mui/utils/elementTypeAcceptingRef/elementTypeAcceptingRef.d.ts:1:8 - error TS1192: Module '"/path/to/sandbox/material-ui/material-ui-tsc-fix/node_modules/@types/prop-types/index"' has no default export.

1 import PropTypes from 'prop-types';
         ~~~~~~~~~

node_modules/@mui/utils/integerPropType/integerPropType.d.ts:1:8 - error TS1192: Module '"/path/to/sandbox/material-ui/material-ui-tsc-fix/node_modules/@types/prop-types/index"' has no default export.

1 import PropTypes from 'prop-types';
         ~~~~~~~~~

node_modules/@mui/utils/refType/refType.d.ts:1:8 - error TS1192: Module '"/path/to/sandbox/material-ui/material-ui-tsc-fix/node_modules/@types/prop-types/index"' has no default export.

1 import PropTypes from 'prop-types';
         ~~~~~~~~~

Found 16 errors in 16 files.

Errors  Files
     1  node_modules/@mui/system/borders/index.d.ts:1
     1  node_modules/@mui/system/cssGrid/index.d.ts:1
     1  node_modules/@mui/system/display/index.d.ts:1
     1  node_modules/@mui/system/flexbox/index.d.ts:1
     1  node_modules/@mui/system/palette/index.d.ts:1
     1  node_modules/@mui/system/positions/index.d.ts:1
     1  node_modules/@mui/system/shadows/index.d.ts:1
     1  node_modules/@mui/system/sizing/index.d.ts:1
     1  node_modules/@mui/system/spacing/index.d.ts:1
     1  node_modules/@mui/system/style/index.d.ts:1
     1  node_modules/@mui/system/typography/index.d.ts:1
     1  node_modules/@mui/utils/chainPropTypes/chainPropTypes.d.ts:1
     1  node_modules/@mui/utils/elementAcceptingRef/elementAcceptingRef.d.ts:1
     1  node_modules/@mui/utils/elementTypeAcceptingRef/elementTypeAcceptingRef.d.ts:1
     1  node_modules/@mui/utils/integerPropType/integerPropType.d.ts:1
     1  node_modules/@mui/utils/refType/refType.d.ts:1
  ~~~~~~~~~~~~~~~~~~~

Consequently, I encounter an issues documented at https://github.com/mui/material-ui/issues/43700 & https://github.com/mui/material-ui/issues/43685

Janpot commented 1 month ago

Assuming a bundler will be consuming your library, you should be able to set "allowSyntheticDefaultImports": true in your tsconfig.json. We can work towards removing this setting in our tsconfig files to improve our internal import behavior, but at the moment we won't fully be able to as we have the previously demonstrated prop-types mismatch with its typings package.

morozow commented 1 month ago

@Janpot I’m pleased that it’s working. If there’s a need for documentation improvement, here’s the PR: https://github.com/mui/material-ui/pull/43747 Thank you!

Janpot commented 1 month ago

@morozow I'm glad it works for you. Thank you for improving the docs. Is this issue sufficiently addressed for you so that we can close this ticket and the related ones?

We will have to revise our imports as part of https://github.com/mui/material-ui/pull/43264. I'm going to improve/fix the node-esm runtime tests before we touch those imports.

morozow commented 1 month ago

@Janpot The problem has been fixed by updating the documentation: https://github.com/mui/material-ui/pull/43747

github-actions[bot] commented 1 month ago

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

[!NOTE] We value your feedback @morozow! How was your experience with our support team? If you could spare a moment, we'd love to hear your thoughts in this brief Support Satisfaction survey. Your insights help us improve!