Repository navigation
[CDX-619] Align browse types with JS SDK - #273
TarekAlQaddy wants to merge 1 commit into
Conversation
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
| browse_filter_value: string; | ||
| filter_match_types: Record<string, any>; | ||
| filters: Record<string, any>; | ||
| filter_match_types?: Record<string, any>; |
There was a problem hiding this comment.
Important Issue: BrowseRequestType.filter_match_types is typed as Record<string, any> (the server-echoed request field), while the input parameter BrowseParameters.filterMatchTypes is correctly typed as Record<string, 'all' | 'any' | 'none'>. These should be consistent. Since this field is echoed back from the server, it should be Record<string, 'all' | 'any' | 'none'> here too, to allow callers to read the echoed value with the same precision.
Suggested fix:
filter_match_types?: Record<string, 'all' | 'any' | 'none'>;| total_num_results: number; | ||
| features: Partial<Feature>[]; | ||
| related_searches?: Record<string, any>[]; | ||
| related_browse_pages?: Record<string, any>[]; |
There was a problem hiding this comment.
Suggestion: related_searches and related_browse_pages are added to the Response (search) interface as Record<string, any>[]. These same fields are also added to GetBrowseResultsResponseData in the same fashion. If the shapes of these objects are known (e.g. { term: string } for related_searches, and { filter_name: string; filter_value: string } for related_browse_pages), defining dedicated interfaces would provide better type safety and IDE auto-completion. At minimum, these could be extracted to shared, named interfaces in index.d.ts to avoid duplication between the search and browse types.
| export * from './tracker'; | ||
| export * from './searchandising'; | ||
|
|
||
| type RequireAtLeastOne<T, Keys extends keyof T = keyof T> = |
There was a problem hiding this comment.
Important Issue: RequireAtLeastOne is defined as a non-exported type (lowercase type, no export). However, FilterBy which depends on it is exported. This works because the utility type is only needed internally for type-level computation, but it creates an inconsistency: the utility type is invisible to consumers who might want to use it directly. If intentionally unexported, a comment explaining the decision would help future maintainers. If it should be reusable, add export.
Also note the utility type is defined after the export * from '...' barrel re-exports — if any of the re-exported modules also need this type (they currently do not), it would not be available. Consider moving it before the re-exports or at least before its first usage.
| fetch: fetchSpy, | ||
| }); | ||
|
|
||
| browse.getBrowseResults(filterName, filterValue, { filters, filterMatchTypes }).then((res) => { |
There was a problem hiding this comment.
Suggestion: The new integration tests for filterMatchTypes use promise .then() + done callback style, which is the existing pattern in this file. However, these tests are missing rejection handling — if the promise rejects the test will time out rather than fail with a useful error message. The surrounding existing tests share this pattern, but since new tests are being added, consider adding a .catch(done) or switching to async/await with try/catch to align with more modern patterns and provide better failure diagnostics:
browse.getBrowseResults(filterName, filterValue, { filters, filterMatchTypes })
.then((res) => {
// assertions...
done();
})
.catch(done);| filterMatchTypes: { size: 'some' }, | ||
| }); | ||
|
|
||
| expectNotAssignable<GetBrowseResultsResponse>({ |
There was a problem hiding this comment.
Suggestion: The expectNotAssignable<GetBrowseResultsResponse> test checks that an object missing result_id is not assignable. This is a good test for the breaking change introduced by making result_id required. However, there is no corresponding expectNotAssignable test in src/types/tests/search.test-d.ts for SearchResponse, even though SearchResponse already had request, response, and result_id as required fields before this PR. For consistency and to guard against future regressions, a parallel test in search.test-d.ts would be valuable.
There was a problem hiding this comment.
🟢 Approval recommended
The type and runtime changes are consistent with the stated compatibility scope and have appropriate coverage.
0 open findings
What changed in this PR
Aligns Node SDK browse/search typings with the JavaScript SDK and adds filterMatchTypes request support.
Changes:
- Adds and tests
filter_match_typesserialization. - Refines browse responses and variations-map typings.
- Corrects an ESLint suppression directive.
| File | Description |
|---|---|
src/types/tests/types.test-d.ts |
Tests variations-map types. |
src/types/tests/browse.test-d.ts |
Tests browse type changes. |
src/types/search.d.ts |
Adds search parameter and response fields. |
src/types/index.d.ts |
Expands variations-map models. |
src/types/browse.d.ts |
Aligns browse request and response types. |
src/modules/search.js |
Sends search filter match types. |
src/modules/browse.js |
Sends browse filter match types. |
spec/src/modules/search.js |
Tests search serialization. |
spec/src/modules/catalog/catalog-facet-configurations-v2.js |
Fixes ESLint suppression scope. |
spec/src/modules/browse.js |
Tests browse serialization. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Related to #268
This PR aligns the types with the JS SDK which has more accurate typings. This also adds
filter_match_typesto both search and browse.Implemented all the changes that look safe and wouldn't break any existing code except for rarely used or unrealistic cases.
Compatibility
Nothing changes at runtime except that
filterMatchTypesis now sent. Calling the SDK with any parameters that work today still compiles, and so does code that checks optional fields with?.orif. The type-level edge cases are:request,responseorresult_idrelated_searches/related_browse_pagesare read without a check, or used as a non-array (they wereany)?./ fix the typecollection,variations,variations_map,filter_match_types,filtersorqueryare read without a check on a variable typed with the named type (GetBrowseResultsResponseData,BrowseResultData,BrowseRequestType)?.or a defaultaggregationvalue is assigned to the old four-value type, or used in an exhaustiveswitchReads through a browse response object (
res.response.collection,res.request.filters, …) are unaffected, becauserequestandresponseare stillPartial<>.Deferred (breaking, for a major release)
VariationsMap.group_byoptional.BrowseResultData.variations_maptyped asVariationsMapResponse(unknownvalues instead ofany).Partial<>fromrequest/responseand their lists (result_sources,facets,groups,results,sort_options,features). Full model sharing with the JS SDK depends on this.BrowseRequestType.facet_name.