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

Move 'authoritative copy' of Material-UI Typescript Definitions to Material-UI Repository #2406

Closed

Conversation

stephenjelfs
Copy link

This pull request moves the 'authoritative copy' of Material-UI Typescript Definitions to the main Material-UI Repository (see also #2392 for the motivation behind this).

…terial-UI Repository

Copied material-ui definitions and tests from definitivelyTyped.
Installed required react definitions.
Added gulp task to 'test' the definitions.
Updated package.json so npm runs the tests,
Added location to package.json so that 'tsd link' can find the
definitions.
@alitaheri
Copy link
Member

@stephenjelfs I think breaking this file down and simply add *.d.ts corresponding to each component would do. This seems too hacky. besides, if all of it is in one single file maintaining it will be hard. with *.d.ts we can loosely couple components with their typings. Thoughts @oliviertassinari @newoga @shaurya947 ?

@stephenjelfs
Copy link
Author

@subjectix Breaking this file up sounds like a nice idea--with a build task to automatically create a nice easy to use 'bundle' at the end?

@alitaheri
Copy link
Member

@stephenjelfs bundling them would be as simple as a concat, I like your idea 👍 👍

@stephenjelfs
Copy link
Author

@subjectix Automatically generating a bundle for library users (that doesn't expose too many internals) might not be as simple as concat microsoft/TypeScript#2568.

@alitaheri
Copy link
Member

well if each d.ts file only has a module declaration I think it can be that simple. just like this section. They are all in a single d.ts file. all the classes and interfaces will be declared in the same place. I think it's rather easy for this library.

Edit:

When I get home tonight, I'll provide a sample to tackle with this, and we'll see if this can work out. what do you say @oliviertassinari ?

@oliviertassinari
Copy link
Member

Merging this PR would mean that we maintain the typescript definition.
But looking at the PR diff lines: +5,181 −1,
I don't think that it's a good idea without a way to generate this definition automatically from the PropType.

Automatically generating a bundle for library users (that doesn't expose too many internals) might not be as simple as concat microsoft/TypeScript#2568.

Maybe, we should wait this issue to be resolved?

@stephenjelfs
Copy link
Author

@oliviertassinari I agree--maybe we should wait for a nicer solution--shall I go ahead and close this issue?

@oliviertassinari
Copy link
Member

@stephenjelfs Yes, I think that we should wait for a better solution.
@subjectix Are you ok with closing this PR?

@alitaheri
Copy link
Member

@oliviertassinari Yeah, go on. I'm ok with it.

@bb
Copy link

bb commented Sep 23, 2016

Hi @oliviertassinari and @stephenjelfs, microsoft/TypeScript#2568 was resolved/fixed. Could this be reopened now or are there other open issues which this depends on?

It looks like material-ui is not working out of the box yet with Typescript (2.0) and I think it would be nice if the two played well together.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 23, 2016

@bb I'm not familiar with TypeScript. What's the implication of this issue behind resolved?
From my point of view, having a TypeScripe definition, as a Flow one, would be really great as long as we can autogenerate it.

@bb
Copy link

bb commented Sep 25, 2016

@oliviertassinari honestly, I don't exactly know. I was asking because you and @stephenjelfs referring to it in the comments above.

Material-ui not working out of the box was an error on my side: I used a react+typescript boilerplate which had react typings installed but obviously not material-ui. After installing the typings it worked fine.

However, it would be nicer, if material-ui worked without this extra step by delivering the typings directly.

@Xstoudi
Copy link

Xstoudi commented Dec 29, 2016

Hey guys!
The current definitions on the DefinitelyTyped repo are often outdated, specially because MaterialUI is evolving day by day.

Currently, the definitions on DefinitelyTyped/DefinitelyTyped are outdated (written for TypeScript 1.0) and incomplete (new features of MaterialUI aren't supported).

The main advantage of having the definitions bundled with NPM package is that you provide strong, complete and up to date definitions for those who use a React-TypeScript-MaterialUI combination.

You can generate definitions automatically only if your project is developped in TypeScript with tsc --declaration but I doubt it is on your roadmap.

Whatever, the fact is that create a workgroup of 2-3 contributors who rewrites and maintains an official version of MaterialUI TypeScript Definitions would be really great.

@Xstoudi
Copy link

Xstoudi commented Dec 29, 2016

Actually this PR is outdated too: Typings is no longer used to manage definitions files since TypeScript provides a solution that only uses NPM.

@oliviertassinari
Copy link
Member

The current definitions on the DefinitelyTyped repo are often outdated

That's our main issue with providing a Typescript definition of the components. The very fact that it's outdated makes me believe that we can't auto-generate the definitions. Hence, requiring time. That's time we can't afford to spend.

If somebody is coming with a PR to autogenerate the documentation we would love to merge it.
Otherwise, I think that we would be better of letting the community handle a definition.
By the way, we have started using flow on the next branch. I definitely see some value in a typed language.

@Xstoudi
Copy link

Xstoudi commented Dec 30, 2016

We can't generate TypeScript definitions from Javascript modules because TypeScript is a superset of Javascript.

By the way, we have started using flow on the next branch.

Using what ?

I definitely see some value in a typed language.

If you used ES6, your code is really close of what he would be, written in TypeScript. Don't think it would take very long to pass from one to the other.

Just ask if you have some questions.

@oliviertassinari
Copy link
Member

Using what ?

https://flowtype.org/

@Xstoudi
Copy link

Xstoudi commented Dec 30, 2016

Mmmh, looks interesting, I think it's possible to use Flow and TypeScript:
TypeScript as typed language and Flow to avoid potential errors on execution.

@zannager zannager added the package: material-ui Specific to @mui/material label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants