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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

lewxdev
Copy link

@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 f4bc9a0

@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
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
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
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?

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
4 participants