Fix: Browse and Search types #410
Conversation
xkr47
force-pushed
the
fix/browse-and-search-types
branch
from
601719e to
09bcbaa
Compare
| export interface BrowseResponse<ResponseType, OmittedRequestFields extends keyof BrowseRequestType> | ||
| extends Record<string, any> { | ||
|
|
||
| request: Omit<BrowseRequestType, OmittedRequestFields>; |
There was a problem hiding this comment.
Yeah I know the documentation says the format of the request object should not be programmatically relied on.. but otoh you have declared a type for the fields here, so I improved them at least a little bit..
| >; | ||
| export type GetBrowseFacetOptionsResponse = BrowseResponse< | ||
| Pick<GetBrowseResultsResponseData, 'facets'> | ||
| Pick<GetBrowseResultsResponseData, 'facets' | 'total_num_results'>, |
There was a problem hiding this comment.
The total_num_results is indeed included here as well
| Pick<GetBrowseResultsResponseData, 'facets' | 'total_num_results'> | ||
| GetBrowseFacetsResultsResponseData, |
There was a problem hiding this comment.
The facets response actually returns a lighter version of the facet data and therefore I made a separate interface for it.
| } | ||
|
|
||
| export interface GetBrowseFacetsResultsResponseData extends Record<string, any> { | ||
| facets: BrowseFacet[]; |
There was a problem hiding this comment.
I defined the light-weight BrowseFacet in index.d.ts, thought it was more in-context there even though it is a browse-specific type..
| filter_match_types?: Record<string, any>; | ||
| filters?: Record<string, any>; | ||
| fmt_options: FmtOptions; | ||
| facet_name: string; |
There was a problem hiding this comment.
facet_name comes from /browse/facet_options endpoint
| } | ||
|
|
||
| export interface HierarchialFacetOption extends FacetOption { | ||
| options?: HierarchialFacetOption[]; |
There was a problem hiding this comment.
Can the hierarchial facet option be recursive to to any depth? This typing assumes so. If it only allows two levels deep options, the type here should be replaced with FacetOption.
Also did not test this option so I don't know if it is always present or optional, so left it optional.
| variations: Record<string, any>[]; | ||
| variations_map: Record<string, any> | Record<string, any>[]; | ||
| } | ||
| export type Result = BrowseResultData; |
There was a problem hiding this comment.
The Result was identical so I thought we could just alias it here.
|
Hey @xkr47 , thanks for your contribution! We will review and get these merged |
2 participants
Hi,
For at least the Search and Browse apis, there were a LOT of usage of
Partial<...>when in fact almost all fields seem to always be present. I checked the api responses and also checked the online api documentation and these are the changes I feel pretty confident with. I hope you can consider at least most of these changes. I look forward to any feedback as well.Instead of using Partial I tried to make only select fields optional using the
?:syntax. Also I split some interfaces into separate more specific ones to be able to provide better typings in all scenarios.