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

ExpansionPanel doesn't support ref in typescript description #14295

Closed
2 tasks done
indapublic opened this issue Jan 24, 2019 · 6 comments
Closed
2 tasks done

ExpansionPanel doesn't support ref in typescript description #14295

indapublic opened this issue Jan 24, 2019 · 6 comments
Milestone

Comments

@indapublic
Copy link

Ref callback is successfully working for ExpansionPanel but ExpansionPanelProps not containing ref method declaration in https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/ExpansionPanel/ExpansionPanel.d.ts

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

No errors.

Current Behavior 😯

I see error:

Type '{ children: Element; expanded: false; ref: ((ref?: any) => void) | undefined; }' is not assignable to type 'IntrinsicAttributes & ExpansionPanelProps & { children?: ReactNode; }'.
Property 'ref' does not exist on type 'IntrinsicAttributes & ExpansionPanelProps & { children?: ReactNode; }'.ts(2322)

Steps to Reproduce 🕹

Put code

<ExpansionPanel expanded={false} ref={ref}>
...
</ExpansionPanel>

and run linter.

Tech Version
Material-UI v3.1.0
React 16.5.2
Browser
TypeScript 2.8.3
etc.
@eps1lon
Copy link
Member

eps1lon commented Jan 29, 2019

I'm not sure what the public API is here. Our type declarations imply that you should assume that our components can't hold refs. They can be class components (can have a ref) or function components (can't have a ref). It is working at run-time because most of our components are class components.

For all components innerRef is supported in typescript and bypasses the styling HOC. However some of the inner components are not class components which might result in a runtime error.

I would recommend you do not rely on a ref prop since it is not documented at the moment. Right now the safest thing to do is to use the RootRef component.

@oliviertassinari
What is the official stance on ref props? Do we support this for all of our components or should we add explicit documentation to the components where we do support it?

@oliviertassinari
Copy link
Member

@eps1lon As you said, some component supports the ref, some don't. It's the current state of affairs.
I believe that supporting the ref property for all our components requires the usage of the React.forwardRef API. I'm not sure it's a good tradeoff. It's manual labor everywhere. Something like reactjs/rfcs#97 can better scale.

@eps1lon
Copy link
Member

eps1lon commented Jan 29, 2019

@eps1lon As you said, some component supports the ref, some don't. It's the current state of affairs.

How do you want to document this? It's currently not obvious what the public API concerning refs is. Should we make it implicit with our type declarations? If so are you ok with the current state that we disallow refs on all of our components? Explicit by adding it to the API section of each component?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 29, 2019

@eps1lon From a documentation perspective, we can use the following logic:
support ref = withStyles or React.Component found to generate the API docs automatically.

But I think that it's simpler if people use <RootRef> when the need to access the underlying DOM node or the existing inputRef / buttonRef properties. By not supporting the ref, we allow ourselves to:

  • Prevent people from accessing class instance methods
  • Allow ourselves to refactor the internal
  • Potentially fit in the future of React Fragment refs RFC reactjs/rfcs#97. It removes the need of using React.forwardRef everywhere as findDOMNode is getting deprecated.

So, I'm tempted to say that the TypeScript definitions are correct. How do you want to handle the problem?

@eps1lon
Copy link
Member

eps1lon commented Jan 29, 2019

So, I'm tempted to say that the TypeScript definitions are correct. How do you want to handle the problem?

This is fine by me. If we can narrow them down to function components i.e. no refs then we automatically resolve #14191 WRT to styled-components.

The issue is that we're relying on refs in our own codebase for Paper, Backdrop etc (see #13722). Internally this is fine but we're also doing this in our demos. Not sure if there is much value in collecting all the components where we use refs ourselves and "enable" them in typescript. Might be easier to just declare every component decorated with withStyles as a ComponentClass.

@eps1lon eps1lon added this to the v4 milestone Mar 12, 2019
@eps1lon
Copy link
Member

eps1lon commented Mar 15, 2019

Closing this in favor of #14415. We will have proper ref documentation and typing once #14415 is closed.

@eps1lon eps1lon closed this as completed Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants