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

[useAutocomplete] Improve TS typing of groupedOptions prop #44657

Merged
merged 16 commits into from
Dec 18, 2024

Conversation

lewxdev
Copy link
Contributor

@lewxdev lewxdev commented Dec 5, 2024

resolves #40739

@mui-bot
Copy link

mui-bot commented Dec 5, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against a3adeea

@mj12albert mj12albert changed the title [mui-base][useAutocomplete] Improve typing UseAutocompleteReturnValue["groupedOptions"] [useAutocomplete] Improve typing UseAutocompleteReturnValue["groupedOptions"] Dec 6, 2024
@mj12albert
Copy link
Member

@lewxdev Hey thanks for the PR, we are no longer maintaining @mui/base here (and the API is going through a huge overhaul)

However useAutocomplete is moved to Material UI in v6 here where it continues to receives support and fixes

Would you mind making the change to that file instead 😬

@mj12albert mj12albert added typescript component: autocomplete This is the name of the generic UI component, not the React module! labels Dec 6, 2024
@zannager zannager requested a review from mj12albert December 6, 2024 18:10
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@lewxdev What issue are facing by directly passing it through UseAutocompleteProps which you proposed in the issue?

@lewxdev
Copy link
Contributor Author

lewxdev commented Dec 11, 2024

@ZeeshanTamboli because groupBy is a function (unlike the other generic props), I had trouble getting the option param to be inferred by typescript..
the proposed solution worked when not using the option param or when manually typing the option param
see this playground

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@lewxdev What about the following changes:

--- a/packages/mui-material/src/useAutocomplete/useAutocomplete.d.ts
+++ b/packages/mui-material/src/useAutocomplete/useAutocomplete.d.ts
@@ -373,14 +373,6 @@ export function useAutocomplete<
     groupBy?: undefined;
   },
 ): UseAutocompleteReturnValue<Value, Multiple, DisableClearable, FreeSolo, false>;
-export function useAutocomplete<
-  Value,
-  Multiple extends boolean | undefined = false,
-  DisableClearable extends boolean | undefined = false,
-  FreeSolo extends boolean | undefined = false,
->(
-  props: UseAutocompleteProps<Value, Multiple, DisableClearable, FreeSolo>,
-): UseAutocompleteReturnValue<Value, Multiple, DisableClearable, FreeSolo, unknown>;

 export interface UseAutocompleteRenderedOption<Value> {
   option: Value;
@@ -392,7 +384,7 @@ export interface UseAutocompleteReturnValue<
   Multiple extends boolean | undefined = false,
   DisableClearable extends boolean | undefined = false,
   FreeSolo extends boolean | undefined = false,
-  HasGroupBy extends boolean | unknown = false,
+  HasGroupBy extends boolean = false,
 > {
   /**
    * Resolver for the root slot's props.
@@ -483,11 +475,7 @@ export interface UseAutocompleteReturnValue<
   /**
    * The options to render. It's either `Value[]` or `AutocompleteGroupedOption<Value>[]` if the groupBy prop is pr
ovided.
    */
-  groupedOptions: HasGroupBy extends true
-    ? AutocompleteGroupedOption<Value>[]
-    : HasGroupBy extends false
-      ? Value[]
-      : Value[] | AutocompleteGroupedOption<Value>[];
+  groupedOptions: HasGroupBy extends true ? AutocompleteGroupedOption<Value>[] : Value[];
 }

 export default useAutocomplete;

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

We will also need to change the description of groupedOptions here. My suggestion:

/**
 * The options to render.
 * - If `groupBy` is provided, the options are grouped and represented as `AutocompleteGroupedOption<Value>[]`.
 * - Otherwise, the options are represented as a flat array of `Value[]`.
 */

@ZeeshanTamboli ZeeshanTamboli changed the title [useAutocomplete] Improve typing UseAutocompleteReturnValue["groupedOptions"] [useAutocomplete] Improve typing of groupedOptions type Dec 12, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [useAutocomplete] Improve typing of groupedOptions type [useAutocomplete] Improve TS typing of groupedOptions prop Dec 12, 2024
@lewxdev
Copy link
Contributor Author

lewxdev commented Dec 12, 2024

@ZeeshanTamboli the issue with the first proposed change is if someone passes an option to groupBy that can't be narrowed (e.g. the input type is ((option: Value) => string) | undefined), it will give a potentially incorrect type for groupedOptions.

options:

  • accept the proposed change as potentially incorrect
  • accept only exactly undefined OR ((option: Value) => string) (likely requires a utility type or complex typing that may make the overall type more confusing)

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Dec 13, 2024

@ZeeshanTamboli the issue with the first proposed change is if someone passes an option to groupBy that can't be narrowed (e.g. the input type is ((option: Value) => string) | undefined), it will give a potentially incorrect type for groupedOptions.

@lewxdev I didn’t fully understand this. Does the issue come from the proposed change I mentioned here? Could you provide a TS playground example to explain?

@lewxdev
Copy link
Contributor Author

lewxdev commented Dec 13, 2024

@ZeeshanTamboli ah, that was a misunderstanding on my part. I've not worked with declaration files in this way before and thought the overload required an implementation signature.

after looking into the documentation on overloaded function declarations, I understand your proposed change.

all proposed changes have been made

@ZeeshanTamboli
Copy link
Member

@lewxdev I noticed the groupBy type in IntelliSense appears like this:

Screenshot (36)

This might be caused by intersecting types in the overload. Can we correct it?

@lewxdev
Copy link
Contributor Author

lewxdev commented Dec 17, 2024

@ZeeshanTamboli I believe I've corrected it. I've used Omit and PartiallyRequired from '@mui/types' to update the function overloads, hopefully this works

Screenshot 2024-12-17 at 06 48 31

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@lewxdev I updated the demos to use useAutocomplete from Material UI and removed type assertions since the type is now conditional. Looks good, thanks!

@ZeeshanTamboli ZeeshanTamboli merged commit 1920b80 into mui:master Dec 18, 2024
22 checks passed
@maapteh
Copy link

maapteh commented Jan 17, 2025

why make an optional prop in minor release suddenly required ...

@ZeeshanTamboli
Copy link
Member

why make an optional prop in minor release suddenly required ...

It isn't required. Replied in #45044.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: autocomplete This is the name of the generic UI component, not the React module! typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[useAutocomplete] Improve typeof groupOptions in useAutocomplete
5 participants