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] Improve type coverage of colDef #2188

Merged
merged 16 commits into from
Aug 9, 2021

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jul 22, 2021

Fixes #2187

Other public methods that are not typed

@flaviendelangle flaviendelangle marked this pull request as draft July 22, 2021 09:54
@flaviendelangle flaviendelangle self-assigned this Jul 22, 2021
@flaviendelangle flaviendelangle added breaking change on hold There is a blocker, we need to wait typescript labels Jul 22, 2021
@flaviendelangle flaviendelangle removed the on hold There is a blocker, we need to wait label Jul 22, 2021
@flaviendelangle
Copy link
Member Author

@m4theushw I removed the event and props on some of the custom cell renderers because the typing says that they should not be here.
I would like your confirmation on this point 👍

@flaviendelangle
Copy link
Member Author

Follow up on #2189 (comment)
If we want to stop exposing apiRef, I suppose that we don't want to type it on the public methods while it's still their.
The columns / colDef typing is still relevant I think.

@flaviendelangle flaviendelangle marked this pull request as ready for review July 23, 2021 08:00
@dtassone dtassone changed the title [typescript] Avoid untyped api and colRef properties [typescript] Avoid untyped api and colDef properties Jul 23, 2021
@oliviertassinari
Copy link
Member

How did we get away from the circular dependency in the types?

@flaviendelangle
Copy link
Member Author

@oliviertassinari the type-only import ( import type { MyType } ) handles circular imports perfectly from what I read.
I'm not sure if, when importing a type with import { MyType } there is a real circular import issue or if it's just a limitation of the ESLint rule.

microsoft/TypeScript#35200
import-js/eslint-plugin-import#1453

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 26, 2021

@flaviendelangle Ok, I didn't know that TypeScript support circular type declaration. It sounds interesting to give it a try and see if it fly at scale or not. At least, we would have found and fixed other issues in the codebase.

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Jul 26, 2021

I tried to build yarn release:build and the .d.ts files have been generated correctly.

https://github.com/mui-org/material-ui-x/pull/2213/files#diff-b98630eb301ddfb1b291caade7edafda784e434b852f21efc232324774d4857eR37
Here (other PR) I just fixed the circular import rather than adding type to dozens of imports.
But as long as our imports are only types (and if classes / enum, only used as types), this solution should work.

Alternatively, the import('...') syntax also support circular import. But it's heavier

api: import('../api/gridApi').GridApi

@flaviendelangle flaviendelangle changed the title [typescript] Avoid untyped api and colDef properties [typescript] Avoid untyped colDef property Aug 4, 2021
@flaviendelangle flaviendelangle changed the title [typescript] Avoid untyped colDef property [typescript] Avoid untyped colDef and api properties (except for soon to be removed api) Aug 4, 2021
@oliviertassinari oliviertassinari changed the title [typescript] Avoid untyped colDef and api properties (except for soon to be removed api) [typescript] Improve type coverage of colDef Aug 6, 2021
@flaviendelangle flaviendelangle merged commit c4f5b06 into mui:master Aug 9, 2021
@flaviendelangle flaviendelangle deleted the avoid-api-any branch August 9, 2021 08:16
@m4theushw m4theushw mentioned this pull request Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[typescript] Avoid api: any or colRef: any due to circular import
5 participants