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

[material-ui][typescript][InputBase] Fix correct type of InputBaseComponentProps #41165

Closed
wants to merge 1 commit into from

Conversation

dev-natalya
Copy link

@dev-natalya dev-natalya commented Feb 18, 2024

Added missing types to InputBaseComponentProps by extending from React.InputHTMLAttributes instead of React.HTMLAttributes.

Here's a link to React.InputHTMLAttributes from @types/react.

@mui-bot
Copy link

mui-bot commented Feb 18, 2024

Netlify deploy preview

https://deploy-preview-41165--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 2048882

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 18, 2024

It's unclear why we should merge this: DefinitelyTyped/DefinitelyTyped#43985 (why should it be fixed in Material UI and not somewhere else?)

@dev-natalya dev-natalya changed the title [material-ui][types] add enterkeyhint type to `InputBaseComponentPr… [material-ui][types] add missing types to InputBaseComponentProps Feb 18, 2024
@dev-natalya
Copy link
Author

dev-natalya commented Feb 18, 2024

@oliviertassinari thank you and sorry for taking the wrong route to resolve this issue before.
Please re-review; also updated the title and PR's description

@ZeeshanTamboli
Copy link
Member

@DiegoAndai Could you review this PR? The type InputBaseComponentProps is utilized for both the inputProps and inputComponent props. Is there a consideration to deprecate or remove these props, alongside components and componentsProps, for v6/v7? The slotProps.input maintains the correct type of React.InputHTMLAttributes. So, would it be worthwhile to include this type fix until then?

@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][types] add missing types to InputBaseComponentProps [material-ui][typescript][InputBase] Fix correct type of InputBaseComponentProps Mar 5, 2024
@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work package: material-ui Specific to @mui/material component: input This is the name of the generic UI component, not the React module! labels Mar 5, 2024
@DiegoAndai
Copy link
Member

Hey @ZeeshanTamboli, thanks for tagging me and keeping an overview of current initiatives.

Indeed, the inputComponent and inputProps should be deprecated in v6 and eventually removed in v7. Being so close to their deprecation, I wouldn't change their types.

@dev-natalya is using slots.input and/or slotProps.input an alternative for you? If it is, I suggest migrating to those APIs.

@dev-natalya
Copy link
Author

dev-natalya commented Mar 9, 2024

@DiegoAndai let me please rephrase you:

  • PR won't be merged because inputComponent & inputProps are to-be-(deprecated/removed)
    • you don't see value for the type fix for users who won't be able to migrate; we should understand that migration from v5 to v6 won't happen overnight, meaning there'll be a lot of devs/teams, who would postpone migration until at least few days are allocated to it. It's still beneficial to merge this type of fix for the existing users and those who postpone migration to v6.
  • about slots & slotProps; it's a different API and the typings there are already equal to what this PR is proposing
    • I think the fact that the type for slotProps.input is equal to this PR's changes is another argument this PR should be merged, because otherwise, currently, we have a typings mismatch (see below)

slotProps.input's current typings (same as this PR's proposal for inputProps):

input?: React.InputHTMLAttributes<HTMLInputElement> &

@DiegoAndai
Copy link
Member

@dev-natalya this is my reasoning:

There are two types of users here:

  1. Those that use inputComponent/inputProps and don't need this fix
  2. Those that use inputComponent/inputProps and need this fix

Option 1, we merge this, then:

  1. We might break implementation for users of type 1., they have to deal with it right away
  2. We will fix it for users of type 2., but they'll have to migrate eventually anyway

Option 2, we deprecate (which we're doing anyway):

  1. We don't break anything for users of type 1., they get the deprecation warning and can plan their update
  2. We provide a fix for users of type 2., which is migrating to the updated APIs

In both options, users have to eventually migrate, but in option 2, we avoid breaking users' implementations and instead incentivize migrating.

That's why I'm leaning toward not merging, unless migrating to the updated APIs is not an option, that's why I asked if using slots.input and/or slotProps.input is an alternative for you

What do you think?

@dev-natalya
Copy link
Author

dev-natalya commented Mar 12, 2024

@DiegoAndai I appreciate the explanation; I think the common standard in libraries is:

  • if there's a typings bug, a fix is published in the same version

If you follow a different set of standards, please close the PR.
I just ask you to have this consistency across all PRs:

  • never merge typings bugfixes; keep the bugs until the next major version

The only users who might see a typings error after this PR is merged are:

  • users who actually have typings errors, but were not informed about it because of the missing typings

@DiegoAndai
Copy link
Member

I think the common standard in libraries is:

  • if there's a typings bug, a fix is published in the same version

We follow that standard as well. But I'm making an exception because:

  • We're very close to freezing v5 development and releasing v6 alpha versions, so I'm trying to avoid any non-urgent merges to v5 to avoid changes to it after it's frozen.
  • These props will be deprecated very soon, and it doesn't make sense to me to introduce changes to deprecated (or soon-to-be-deprecated) props.
  • These props have equivalent, up-to-date alternatives in slots.input and slotProps.input.

So:

never merge typings bugfixes; keep the bugs until the next major version

Is not something we follow. We will, in most cases, merge types bugfixes. But this case is a rare exception due to the circumstances I described above.

@ZeeshanTamboli
Copy link
Member

I believe we can close this based on the discussion above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: input This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants