mui / material-ui

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

Typescript Types in beta 6 #7853

Closed stevegeek closed 7 years ago

stevegeek commented 7 years ago

Problem description

@sebald In beta.6 the typescript types exist for material-ui (I see the .d.ts file in the installed package directory) however I keep getting definition errors when doing imports with import Paper from 'material-ui/Paper';

error TS7016: Could not find a declaration file for module 'material-ui/Paper'. '...' implicitly has an 'any' type. Try 'npm install @types/material-ui/Paper' if it exists or add a new declaration (.d.ts) file containing 'declare module 'material-ui/Paper';'

Im not sure why, I dont think there is anything strange with my tsconfig

{
  "compilerOptions": {
    "target": "es2017", 
    "module": "commonjs",
    "jsx": "react",              
    "strict": true,               
    "baseUrl": ".",             
    "paths": {
      "*": ["./build-tools/types/*"]
    },                         
    "inlineSourceMap": true,
    "experimentalDecorators": true   
  }
}

Steps to reproduce

I installed beta.6 and removed my local copies of the material-ui typings (up to now Ive been keeping the typings locally)

Versions

sebald commented 7 years ago

@stevegeek is it only the Paper component or others too?

stevegeek commented 7 years ago

@sebald All typings it seems

sebald commented 7 years ago

I could reproduce your issue with a clean setup. Sorry for any inconvenience 😞

So far, the only thing I can tell you is that moving the exact same file to @types/material-ui works fine. Which means TS seems to handle those cases differently. Will try to solve this as soon as I can.

marcusjwhelan commented 7 years ago

@sebald I was getting some acceptable from the package. for example.

 import {Table, TableBody, TableHeader, TableHeaderColumn, TableRow, TableRowColumn} from 'material-ui/Table';

This will have error lines under Table, TableHeader, TableHeaderColumn, TableRowColumn but not the others. But if I remove the/Table,Table` Loses its error line. It seems that colors is completely missing. Also some attributes of items are missing. TableBody's displayRowCheckbox attribute is not declared but stripedRows and showRowHover are.

There seems to be an extreme amount of inconsistencies with the types. To get the minimal typings from the package I have

"types" : [ "material-ui"]

added to my tsconfig.json file.

stevegeek commented 7 years ago

@marcusjwhelan until this is fixed you can just copy the typings file into your local types directory and everything should work just fine

marcusjwhelan commented 7 years ago

@stevegeek I guess I don't have a local types directory. My project is 100% Typescript, so my typings files are generated from my code. I have a couple of open source Typescript projects that I wrote and but I don't have experience with writing a typings file for a JS project.

@sebald I also tried moving the typings file into the @types folder in my node modules however I come up with the same result as before. Was there a major change in the structure of the project since 0.18.0?

stevegeek commented 7 years ago

@marcusjwhelan have a look at https://stackoverflow.com/a/39827904/268602 it might help you set it up

marcusjwhelan commented 7 years ago

@stevegeek like I said I have experience with Typescript and know how to set up my project.

What I am saying is that the typings form @types is very different from the Types that are shipped with this package. Either some parts like colors are missing or not typed the same way.

There would have to be a large amount of missing types. The @types is almost 10k lines while the types shipped with material-ui is only 2.2k

stevegeek commented 7 years ago

@marcusjwhelan Right, the types in DT are for 0.18.x , v1 is completely different , you will need to port your project to the new MUI version

vyrotek commented 7 years ago

Ah, glad I'm not the only one having typing issues. I'm using tsx and VSCode and can't get it to pull the types from the material-ui package. Even moving them to my local code doesn't seem to be picked up anymore like the last version did.

I wonder if it's because of the tests that were recently added. It may be bombing on the 'enzyme' reference and imports. Should that kind of stuff be in the released index.d.ts file?

Would it be possible to release these typings as a matching beta version of @types/material-ui?

marcusjwhelan commented 7 years ago

I guess there are no types for material-ui-icons

stevegeek commented 7 years ago

@marcusjwhelan They are generated automatically by a script, you will find them in the beta.5 tag of the icons package (see for generation script https://github.com/callemall/material-ui/blob/v1-beta/packages/material-ui-icons/create-typings.js)

Though this issue probably also affects those typings , hence why they are not working from the npm installed package

marcusjwhelan commented 7 years ago

@stevegeek So I need to git clone beta.5 and run this script to get the typings for it? If it is supposed to automatically run then I don't see the output or am I receiving types for it. I am working to fallow the example given on the AppBar example on the .io website.

I see now that the typings are there but very different and able to get the types just fine using the tsconfig which is fine. I did not know there was such a large difference being made.

stevegeek commented 7 years ago

If you install the latest version of https://www.npmjs.com/package/material-ui-icons package (1.0.0-beta.5) the types are included (not sure why icons package isnt on beta.6 like main MUI package). But here is also a relatively recent version of the generated file https://gist.github.com/sebald/c150a718aa05b2206dbdf1ef9e07e9d2

sebald commented 7 years ago

I tried a lot of things, but it seems there is no way to let TS use the material-ui/**/* declaration as the corresponding imports. At least I couldn't find one...

I posted a question to SO. Maybe someone else can help ☹️ https://stackoverflow.com/questions/45804916/typings-for-sub-folders-inside-a-root-index-d-ts


If nothing comes from SO, there are basically two options I see:

(1) generate files that augment mater-uis folder structure and have typings besides the implementation. (2) move the indx.d.ts file to DefinitelyTyped so it is accessible via @types.

I would prefer (2), because most of the work is already done. Although I am not sure how one imports/installs different versions of libraries with the @types.

vyrotek commented 7 years ago

I would prefer (2), because most of the work is already done. Although I am not sure how one imports/installs different versions of libraries with the @types.

I would also definitely prefer having it go through @types / DefinitelyTyped. I've always just imported them through package.json. I'd expect to import it like this "@types/material-ui": "1.0.0-beta.6"

Thanks for looking into all this!

vyrotek commented 7 years ago

@sebald I saw that mhegazy was asking for a contained repro of the issues we're having here https://github.com/Microsoft/TypeScript/issues/17945#issuecomment-323890185.

I quickly put this simple react tsx example together and stumbled onto an interesting situation and wanted to have you take a look. If you feel this represents the problem then feel free to link it to him. https://github.com/vyrotek/material-ui-tsx

I tried adding a button to this component: https://github.com/vyrotek/material-ui-tsx/blob/master/src/App.tsx

import Button from 'material-ui/Button'; VS Code CANNOT find definition for the above import and shows error:

Could not find a declaration file for module 'material-ui/Button'. '/material-ui-tsx/node_modules/material-ui/Button/index.js' implicitly has an 'any' type. Try npm install @types/material-ui/Button if it exists or add a new declaration (.d.ts) file containing declare module 'material-ui/Button';

import { Button } from 'material-ui'; VS Code DOES finds the definition with the above import but I get this error after running npm start:

Failed to compile. \material-ui-tsx\node_modules\material-ui\index.d.ts (2122,25): error TS2307: Cannot find module 'enzyme'.

sebald commented 7 years ago

@vyrotek hey, thank you so much! πŸ™‚ I'll still set my own, (a) because I already had that to trying to resolve the issue, (b) I can make changes to my own repo if the TS team requests any.

The typings require you to have react + enzyme typings installed! Do a yarn add --dev @types/enzyme should fix that.


Even though I prefer (2), I makes my belly aches publishing these typings via @types. Yes, it is the preferred way, I guess. But since every React-related typing also depends on @types/react (d'oh!), you will have different version of the react typings installed (especially if you're using yarn). This causes some funny "multi declaration" errors, which forces you to reinstall all your typings 😐 By publishing the typings with the actual lib and not installing any additional types, we can avoid this issue. A nice side effect is that you'll always have typings that fit your implementation.

stevegeek commented 7 years ago

@vyrotek @sebald Also MS themselves recommend bundling for reasons you stated above and versioning etc - https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html states z

sebald commented 7 years ago

Oh, didn't know that πŸ‘ MS!

vyrotek commented 7 years ago

The typings require you to have react + enzyme typings installed! Do a yarn add --dev @types/enzyme should fix that.

Hmm, that's a first for me. This seems a little unusual. Mostly because people won't know to do this until they try once and get the error. Is there no way to have this dependency automatically bundled?

marcusjwhelan commented 7 years ago

@sebald @vyrotek @stevegeek I have been using the typings just fine. Many projects are build this way. I just simply added node_modules/**/*.d.ts to my "include" and "material-ui" to "types'. Event the Typescript packages I have written work this way.

I am not 100% sure I need the "include" part but I have it anyway. Also @stevegeek thanks for answering me about the project from 0.19 - 1 being very different. Almost done rewriting for V1. Some typing issues come with things like overflow having to be 'auto' | 'hiddine' | and so one instead of string.

@oliviertassinari I am quiet annoyed by the move to withStyles since it seems to break most if not all my styling methods. I have asked elsewhere but how do you make styles dynamic now?

oliviertassinari commented 7 years ago

I have asked elsewhere but how do you make styles dynamic now?

@marcusjwhelan #7408

sebald commented 7 years ago

As already pointed out, to make the typings work add the following to your tsconfig:

{
  "compilerOptions": {
+    "types": ["material-ui"]
  }
}

@oliviertassinari So, after talking to the TS team, the above is their recommended solution, if we leave the typings like that. An even better solution would by to create (what they call) modules. This basically means, like with flow, for every source file, there should be a .d.ts file.

Refactoring the typings like this isn't that much of work. If you think that it's annoying to have these typing files inside src we could also re-create the folder structure in typings and just "merge" it when building.

What do you think? (should I open a seperate issue to discuss this?)

oliviertassinari commented 7 years ago

for every source file, there should be a .d.ts file.

@sebald I don't have any objection to this change. As far as I recall, we went with the single file to ship the typing sooner. If you think that we should break this file into modules, great πŸ‘ .

I see two possible paths going forward. Either we document the compiler options or we create these modules.

sebald commented 7 years ago

@oliviertassinari I think most people will run into issues, because it's not that common to have to set this compiler option. I never had to this in past project and actually thought that if I start to explicitly load types there that I have to do this for every new lib ... 😨

From an end-uster perspective, we should do the refactoring. Hopefully contributor will also see these typings and update them when they make changes :) Who knows!?

I'll make some room tomorrow to apply the changes. Sorry again everyone reading here. Hope you didn't have too much trouble with the messed up typings :-x

54vanya commented 7 years ago

When using withStyles as decorator having this error Error:(70, 1) TS1238:Unable to resolve signature of class decorator when called as an expression. Type 'ComponentClass<{}>' is not assignable to type 'typeof AppContainer'. Type 'Component<{}, ComponentState>' is not assignable to type 'AppContainer'. Types of property 'componentDidMount' are incompatible. Type '(() => void) | undefined' is not assignable to type '() => void'. Type 'undefined' is not assignable to type '() => void'.

stevegeek commented 7 years ago

@sebald No prob, the typings are amazing otherwise :)!

sebald commented 7 years ago

@54vanya please open an issue with a full report. This is not enough information to reproduce your issue.

phryneas commented 7 years ago

if adding types to the compiler-options is not possible, it also works with a triple-slash directive:

/// <reference types="@types/material-ui" />

at the beginning of any file.