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] Allow custom props for custom inputComponent #12697

Closed
wants to merge 2 commits into from

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Aug 28, 2018

As a followup to #12591 which removed support for custom props when passing custom components. This might fix the issue described in #12691 but without a full repo this might not be the case.

Also improves typings when passing textarea or select.

There is still an issue which causes react to log warnings (unknown prop innerRef) when passing any intrinsic element via inputComponent. But this is a concern for the implementation not typescript.

@eps1lon eps1lon force-pushed the core-fix-input-inconsistent-props branch from 44f3e7d to 53aea79 Compare August 29, 2018 05:43
@pelotom
Copy link
Member

pelotom commented Aug 30, 2018

This is suffers from a version of the problem that stymied #11731: callback props can no longer infer their argument types. So whereas this worked before:

<Input inputProps={{ onFocus(e) {} }} />

with this PR it will no longer be able to infer the type of e and will assign it any (which is a type error if you have noImplicitAny enabled).

I'm a bit torn on this issue; I'd really like to use generics everywhere to accurately capture what happens when you override a component prop, but this is a fairly big backwards-breaking change. My strategy so far has been to wait and see if microsoft/TypeScript#26004 will get fixed in the near term, in which case I think we would be able to work around the issue. What do others think? /cc @mui-org/core-contributors

@eps1lon
Copy link
Member Author

eps1lon commented Aug 31, 2018

Did this work prior to #12591? I tested this locally and while vscode does infer the correct type the compiler does indeed throw no implicit any.

@eps1lon
Copy link
Member Author

eps1lon commented Sep 5, 2018

@pelotom How would you proceed with this change? If we don't merge it we should at least revert #12591.

I still prefer this solution over the previous one. I'd rather have some type-checking that requires me to explicitly type callbacks (which even is a tslint rule) than no type-checking at all which is the case prior to #12591 .

I think reverting #12591 and batching this for v4 is the safest route.

@pelotom
Copy link
Member

pelotom commented Sep 5, 2018

I think reverting #12591 and batching this for v4 is the safest route.

Yeah, I kind of think we should defer this until we can apply the approach consistently across the entire library. I personally would be in favor of requiring type annotations on all callbacks for components with a component override prop, even though it would be a significant backwards-breaking change, because it would be correct. But that may be an ideal of purity not shared by other users of the library 🙂

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.

None yet

3 participants