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

[core] Simplify component type #1552

Merged
merged 2 commits into from May 10, 2021

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented May 2, 2021

This is a follow-up on mui/material-ui#26081. Base on the type definitions we have the following:

  • React.ElementType: react hosts component + custom components
  • React.ComponentType: custom components
  • React.JSXElementConstructor: React.ComponentType without the noise (.propTypes, .contextType, .displayName, etc.)

@oliviertassinari oliviertassinari added core Infrastructure work going on behind the scenes typescript labels May 2, 2021
| <span class="prop-name">NoRowsOverlay</span> | <span class="prop-type">React.ElementType</span> | <span class="prop-type">NoRowsOverlay</span> | No rows overlay component rendered when the grid has no rows.|
| <span class="prop-name">Pagination</span> | <span class="prop-type">React.ElementType</span> | <span class="prop-type">Pagination</span> | Pagination component rendered in the grid footer by default.|
| <span class="prop-name">Panel</span> | <span class="prop-type">React.ElementType&lt;GridPanelProps></span> | <span class="prop-type">Panel</span> | Panel component wrapping the filters and columns panels. |
| <span class="prop-name">ColumnMenu</span> | <span class="prop-type">elementType&lt;GridColumnMenuProps></span> | <span class="prop-type">GridColumnMenu</span> | Column menu component rendered by clicking on the 3 dots "kebab" icon in column headers.|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? Not sure I understand the type now...
Why not JSXElementConstructor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have asked a similar question in mui/material-ui#26081 (comment). I would argue that it should be:

Suggested change
| <span class="prop-name">ColumnMenu</span> | <span class="prop-type">elementType&lt;GridColumnMenuProps></span> | <span class="prop-type">GridColumnMenu</span> | Column menu component rendered by clicking on the 3 dots "kebab" icon in column headers.|
| <span class="prop-name">ColumnMenu</span> | <span class="prop-type">componentType&lt;GridColumnMenuProps></span> | <span class="prop-type">GridColumnMenu</span> | Column menu component rendered by clicking on the 3 dots "kebab" icon in column headers.|

JSXElementConstructor, a type simplification of ElementType that developers don't need to be aware of.

cc @eps1lon

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also why this casing? elementType for me it's less understandable.
Why not JSX.Element?

Copy link
Member

@eps1lon eps1lon May 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR: We want to use React terminology in documentation not TypeScript terminology. So elementType would be preferrable

Some react terminology:

<Component />
^^^^^^^^^^^^^ JSX Element
 ^^^^^^^^^ Element Type
<div />
^^^^^^^ host element
 ^^^ host component

"components" are element types. React.Suspense is an element type. React.StrictMode is an element type etc.

The terminology in @types/react tries to be close but for historical reasons we can't use the same terms.

type ComponentType<Props> = FunctionComponent<Props> | ClassComponent<Props>;
type ElementType<Props> = ComponentType<Props> | string;
type JSXElementConstructor<Props> = ((props: Props) => JSXElement) | ({ new(props: Props): ClassComponent<Props>;})

React.ComponentType mostly exists for legacy reasons. In earlier TypeScript versions

function Component() {}
Component.propTypes = {};
Component.displayName = 'Foo';

would throw which is why const Component: React.FunctionComponent got popular. As of TypeScript 3.1 React.FunctionComponent is no longer needed and only a convenience type to validate propTypes or automatically get typed children.

Implicit children are problematic however since not every component implements this prop. Validating .propTypes is way too strict. Additionally, React.ClassComponent has no implicit children. So if you type React.ComponentType<Props> and then try to infer the Props later you get Props | Props & { children?: React.ReactNode } which produces nasty error messages and hard to understand error messages.

Hence why we don't want React.ComponentType nor need it.

React.JSXElementConstructor is what we'd like to be element types. Unfortunately TypeScript needs structural information about the possible element types. And right now it only accepts functions or newables (classes) or strings. So we have to create fake types for certain element types (e.g. react built-ins like React.Suspense or React.StrictMode).

React.ElementType is when you have props that are also implemented by host components e.g. React.ElementType<{ value?: string, defaultValue?: string }> makes sense. But React.ElementType<disableSomeBehaviorWhenISaySo?: boolean> does not make sense because there's no host component that'll implement it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so elementType is not correct as it's not a type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtassone We are talking at the terminology level, not the name used by the code modules. See "Since there are more people who know React than people knowing about React+TypeScript we want to use React terminology instead". Your example imports a TypeScript definition.

@eps1lon So If I understand you correctly, there is no terminology to differentiate a component that is a host or a custom component, so we have to default to elementType terminology in the docs? OK, it sounds fair. I asked because I was hoping we could be more granular in the docs (like we are in the type).

Copy link
Member

@eps1lon eps1lon May 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you import elementType?

That's not the question. We're strictly talking about documentation not the name of a specific type. We never intended the documentation to display valid TypeScript. It was always used as React/propTypes terms. Because the runtime validation is not covering the full structure of the type e.g. props shape.

We could debate whether we want to use TypeScript terminology or React/propTypes terminology but TypeScript is just not good enough for documentation purposes. We'd like to simplify that because all of these terms aren't trivial to differentiate (I know that from experience maintaining @types/react and react-typescript-cheatsheet) but we're blocked by various shortcommings of the TypeScript compiler.

So If I understand you correctly, there is no terminology to differentiate a component that is a host or a custom component, so we have to default to elementType terminology in the docs?

"custom component" would be it but I just find it simpler to use "element type". It may be correct now that all of these are just "custom components" and we could shorthand it with "component" like you'll find in most literature. But it's just not clear to me that this actually applies everywhere and how we would validate that when generating the docs.

In the end I just want to note that automatically generated docs aren't perfect and that we have to cut corners at some point. This seems like a good example of that.

Maybe the best path forward is to create a glossary where we describe the terms used and link to that. That may have the biggest impact compared to the effort required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the best path forward is to create a glossary where we describe the terms used and link to that. That may have the biggest impact compared to the effort required.

Yes that could be useful as I believe other ppl will be confused like me. Maybe renaming the column could also help

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtassone
Copy link
Member

@oliviertassinari I approve but I don't really support the elementType in the doc for the reason mention above.
I think adding it to the glossary could help but still not 100% convinced

@oliviertassinari oliviertassinari merged commit 492c104 into mui:master May 10, 2021
@oliviertassinari oliviertassinari deleted the type-component branch May 10, 2021 09:55
@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 10, 2021

@dtassone Alright, at least, we all agree on the TypeScript update. As for what we use in the documentation, with the auto docs generation, we would get the same diff, without control in this repo, so this is a solid improvement.

For a possible follow-up for the docs, from what I understand, the difference of vision is on: Should we use the React terminology for the type or the TypeScript terminology? I & Seb defends React, you defend TypeScript.
Is it accurate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants