Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeScript errors upgrading MUI from 4.9.5 to 4.9.9 #20441

Closed
2 tasks done
NMinhNguyen opened this issue Apr 6, 2020 · 13 comments · Fixed by #20443 or #20450
Closed
2 tasks done

TypeScript errors upgrading MUI from 4.9.5 to 4.9.9 #20441

NMinhNguyen opened this issue Apr 6, 2020 · 13 comments · Fixed by #20443 or #20450

Comments

@NMinhNguyen
Copy link
Contributor

  • 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 😯

I have been able to trace the following errors to some changes made recently to use TS features such as Omit which aren't available in 3.2.4 (as far as I understand, that's the minimum TS version that MUI supports) or trailing commas:

Click to expand
node_modules/@material-ui/core/Backdrop/Backdrop.d.ts:8:52 - error TS2304: Cannot find name 'Omit'.

8     React.HTMLAttributes<HTMLDivElement> & Partial<Omit<FadeProps, 'children'>>,
                                                     ~~~~

node_modules/@material-ui/core/Box/Box.d.ts:29:22 - error TS1009: Trailing comma not allowed.

29     typeof typography,
                        ~

node_modules/@material-ui/core/Fade/Fade.d.ts:4:36 - error TS2304: Cannot find name 'Omit'.

4 export interface FadeProps extends Omit<TransitionProps, 'children'> {
                                     ~~~~

node_modules/@material-ui/core/TextareaAutosize/TextareaAutosize.d.ts:4:11 - error TS2304: Cannot find name 'Omit'.

4   extends Omit<React.TextareaHTMLAttributes<HTMLTextAreaElement>, 'children' | 'rows'> {
            ~~~~

node_modules/@material-ui/core/styles/shadows.d.ts:26:9 - error TS1009: Trailing comma not allowed.

26   string,
           ~


Expected Behavior 🤔

Steps to Reproduce 🕹

Steps:

  1. Install @material-ui/core@4.9.9
  2. Run your project through tsc using typescript@3.2.4
  3. Observe the errors above

Context 🔦

We set a minimum supported version of TypeScript to 3.2.4 in our design system to match Material-UI but seems like the latest MUI version uses newer TypeScript features. This isn't usually a big issue to React projects, but we also support Angular teams via Custom Elements and usually bumping TypeScript requires them to bump their Angular versions too (usually across major version boundaries) which can be a bit painful. For example, Angular 7.2 supports TS 3.2.4, Angular 8.0.0 adds support for 3.4 dropping support for older versions, Angular 9.0.0 requires 3.6/3.7

Your Environment 🌎

Tech Version
Material-UI v4.9.9
TypeScript v3.2.4
@eps1lon
Copy link
Member

eps1lon commented Apr 6, 2020

Yeah this is problematic. Prettier's trailingCommas: all also applies to TypeScript and I think some trailing commas are only valid in later TS versions.

Maybe trailingCommas: es5 for .d.ts files works.

@NMinhNguyen
Copy link
Contributor Author

Maybe trailingCommas: es5 for .d.ts files works.

I'm happy to work on a PR for this one. What shall we do about Omit? I don't suppose there's a way to lint against such types? :/

@eps1lon
Copy link
Member

eps1lon commented Apr 6, 2020

This was by accident. We have an internal Omit that works without the built-in one.

@NMinhNguyen
Copy link
Contributor Author

Oh nice! But still might be good to be able to lint against using the built-in one. I'll raise a PR shortly for trailing commas and using the internal Omit

@eps1lon
Copy link
Member

eps1lon commented Apr 6, 2020

Yeah this is what we wanted to avoid but I'd argue that this is more of an issue with a PR that was too big not the tooling.

I would rather move forward here though (upcoming @ts-expect-error and tslint -> eslint). Usage numbers for earlier typescript version is pretty low so I don't think it's justified to build tooling for 3.2. But this is for me personally. If wants to build it I'm all ears.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 6, 2020

@eps1lon I wanted to ask about this a couple of days ago, this is an opportunity. What do you think about this diff?

diff --git a/docs/src/pages/getting-started/supported-platforms/supported-platforms.md b/docs/src/pages/getting-started/supported-platforms/supported-platforms.md
index befc4ca39..e49aeba94 100644
--- a/docs/src/pages/getting-started/supported-platforms/supported-platforms.md
+++ b/docs/src/pages/getting-started/supported-platforms/supported-platforms.md
@@ -36,3 +36,9 @@ It's a must-do for static pages, but it needs to be put in balance with not doin

 Material-UI supports the most recent versions of React, starting with ^16.8.0 (the one with the hooks).
 Have a look at the older [versions](https://material-ui.com/versions/) for backward compatibility.
+
+## TypeScript
+
+Material-UI supports the most recent versions of TypeScript.
+However, the codebase doesn't follow semver for types.
+You can expect breaking changes between two minor versions.
+As of now, we support TypeScript ^3.8.0.
+Have a look at the older [versions](https://material-ui.com/versions/) for backward compatibility.
diff --git a/packages/material-ui/package.json b/packages/material-ui/package.json
index e682bb751..0b43f57b5 100644
--- a/packages/material-ui/package.json
+++ b/packages/material-ui/package.json
@@ -39,9 +39,13 @@
   "peerDependencies": {
     "@types/react": "^16.8.6",
     "react": "^16.8.0",
-    "react-dom": "^16.8.0"
+    "react-dom": "^16.8.0",
+    "typescript": "^3.8.0"
   },
   "peerDependenciesMeta": {
+    "typescript": {
+      "optional": true
+    },
     "@types/react": {
       "optional": true
     }

@NMinhNguyen
Copy link
Contributor Author

NMinhNguyen commented Apr 6, 2020

I'm happy to add that as long as we say 3.2.4 😛 (or I can just pass on this breaking change to our Angular users, I don't really care much haha)

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 6, 2020

@NMinhNguyen So they are a couple of topics:

  1. Should we add an optional peer dependency to warn when using a wrong version? Doesn't it even work?
  2. Should we follow semver for our types?
  3. What should be the minimum TypeScript supported version?

@NMinhNguyen
Copy link
Contributor Author

NMinhNguyen commented Apr 6, 2020

  • Should we add an optional peer dependency to warn when using a wrong version? Doesn't it even work?

Not sure how well it works as I haven't used optional peer dependencies, but I'm for it

  • Should we follow semver for our types?

I think (but not 100% sure as I'm not a TS user myself) other packages do tend to follow semver for types, but I'm happy to be corrected!

  • What should be the minimum TypeScript supported version?

Preferably the version that MUI v4 originally said it supported (3.2.4), but I guess that depends on the point above.

@NMinhNguyen
Copy link
Contributor Author

@eps1lon actually I just noticed another error that I initially omitted from the PR description because I thought it was valid, but upon further inspection I see that children became required in TS, but actually became optional in JS: https://github.com/mui-org/material-ui/pull/20298/files#diff-c4e405e74787b5750e61d8305589f825

Should the TS be updated to match JS or the other way around?

@NMinhNguyen
Copy link
Contributor Author

@eps1lon did you see the comment above?

@eps1lon
Copy link
Member

eps1lon commented Apr 7, 2020

@eps1lon did you see the comment above?

I think React.ReactNode includes optional

@NMinhNguyen
Copy link
Contributor Author

NMinhNguyen commented Apr 7, 2020

packages/dialog/examples/BasicDialog.tsx:44:8 - error TS2741: Property 'children' is missing in type '{ open: false; }' but required in type 'DialogProps'.

44       <Dialog open={false} />
          ~~~~~~

  node_modules/@material-ui/core/Dialog/Dialog.d.ts:20:3
    20   children: React.ReactNode;
         ~~~~~~~~
    'children' is declared here.

if I specify children, then the error goes away. I think it's more about the left hand side (i.e. children) than it is about React.ReactNode (although this does include undefined and null) but I could be wrong.

Edit

Updated the error above by just using <Dialog open={false} /> to reduce noise from our own Dialog wrapper

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants