Skip to content

[CDX-619] Align browse types with JS SDK #273

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.

Sign up for GitHub

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

Open
TarekAlQaddy wants to merge 1 commit into
base: master
Choose a base branch
from
+206 −18
Open

[CDX-619] Align browse types with JS SDK #273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions spec/src/modules/browse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,26 @@ describe('ConstructorIO - Browse', () => {
});
});

it('Should return a response with a valid filterName, filterValue, additional filters and filterMatchTypes', (done) => {
const filters = { keywords: ['battery-powered'] };
const filterMatchTypes = { keywords: 'any' };
const { browse } = new ConstructorIO({
...validOptions,
fetch: fetchSpy,
});

browse.getBrowseResults(filterName, filterValue, { filters, filterMatchTypes }).then((res) => {

@constructor-claude-bedrock constructor-claude-bedrock Bot Oct 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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);

const requestedUrlParams = helpers.extractUrlParamsFromFetch(fetchSpy);

expect(res).to.have.property('request').to.be.an('object');
expect(res).to.have.property('response').to.be.an('object');
expect(res).to.have.property('result_id').to.be.an('string');
expect(requestedUrlParams).to.have.property('filter_match_types');
expect(requestedUrlParams.filter_match_types).to.have.property('keywords').to.equal(filterMatchTypes.keywords);
done();
});
});

it('Should return a response with a valid filterName, filterValue and additional fmtOptions', (done) => {
const fmtOptions = { groups_max_depth: 2, groups_start: 'current' };
const { browse } = new ConstructorIO({
Expand Down Expand Up @@ -1053,6 +1073,26 @@ describe('ConstructorIO - Browse', () => {
});
});

it('Should return a response with valid ids, additional filters and filterMatchTypes', (done) => {
const filters = { keywords: ['battery-powered'] };
const filterMatchTypes = { keywords: 'any' };
const { browse } = new ConstructorIO({
...validOptions,
fetch: fetchSpy,
});

browse.getBrowseResultsForItemIds(ids, { filters, filterMatchTypes }).then((res) => {
const requestedUrlParams = helpers.extractUrlParamsFromFetch(fetchSpy);

expect(res).to.have.property('request').to.be.an('object');
expect(res).to.have.property('response').to.be.an('object');
expect(res).to.have.property('result_id').to.be.an('string');
expect(requestedUrlParams).to.have.property('filter_match_types');
expect(requestedUrlParams.filter_match_types).to.have.property('keywords').to.equal(filterMatchTypes.keywords);
done();
});
});

it('Should return a response with valid ids and additional fmtOptions', (done) => {
const fmtOptions = { groups_max_depth: 2, groups_start: 'current' };
const { browse } = new ConstructorIO({
Expand Down
2 changes: 1 addition & 1 deletion spec/src/modules/catalog/catalog-facet-configurations-v2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ describe('ConstructorIO - Catalog', () => {
await catalog.removeFacetConfigurationV2(facetConfig);
} catch (e) {
// Log warning for debugging but don't fail cleanup
// eslint-disable-line no-console
// eslint-disable-next-line no-console
console.warn(`Cleanup warning: failed to remove facet ${facetConfig.name}:`, e.message);
}
// eslint-disable-next-line no-await-in-loop, no-promise-executor-return
Expand Down
24 changes: 24 additions & 0 deletions spec/src/modules/search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,30 @@ describe('ConstructorIO - Search', () => {
});
});

it('Should return a response with a valid query, section, filters and filterMatchTypes', (done) => {
const filters = { keywords: ['battery-powered'] };
const filterMatchTypes = { keywords: 'any' };
const { search } = new ConstructorIO({
...validOptions,
fetch: fetchSpy,
});

search.getSearchResults(query, {
section,
filters,
filterMatchTypes,
}).then((res) => {
const requestedUrlParams = helpers.extractUrlParamsFromFetch(fetchSpy);

expect(res).to.have.property('request').to.be.an('object');
expect(res).to.have.property('response').to.be.an('object');
expect(res).to.have.property('result_id').to.be.an('string');
expect(requestedUrlParams).to.have.property('filter_match_types');
expect(requestedUrlParams.filter_match_types).to.have.property('keywords').to.equal(filterMatchTypes.keywords);
done();
});
});

it('Should return a response with a valid query, section, and fmtOptions', (done) => {
const fmtOptions = { groups_max_depth: 2, groups_start: 'current' };
const { search } = new ConstructorIO({
Expand Down
8 changes: 8 additions & 0 deletions src/modules/browse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ function createQueryParams(parameters, userParameters, options) {
variationsMap,
preFilterExpression,
qsParam,
filterMatchTypes,
} = parameters;

// Pull page from parameters
Expand All @@ -60,6 +61,11 @@ function createQueryParams(parameters, userParameters, options) {
queryParams.filters = filters;
}

// Pull filter match types from parameters
if (filterMatchTypes) {
queryParams.filter_match_types = filterMatchTypes;
}

// Pull sort by from parameters
if (sortBy) {
queryParams.sort_by = sortBy;
Expand Down Expand Up @@ -263,6 +269,7 @@ class Browse {
* @param {string[]} [parameters.hiddenFields] - Hidden metadata fields to return
* @param {string[]} [parameters.hiddenFacets] - Hidden facet fields to return
* @param {object} [parameters.variationsMap] - The variations map object to aggregate variations. Please refer to https://docs.constructor.com/reference/shared-variations-mapping for details
* @param {object} [parameters.filterMatchTypes] - An object specifying whether results must match `all`, `any` or `none` of a given filter
* @param {object} [parameters.preFilterExpression] - Faceting expression to scope search results. Please refer to https://docs.constructor.com/reference/configuration-collections for details
* @param {object} [parameters.qsParam] - Parameters listed above can be serialized into a JSON object and parsed through this parameter. Please refer to https://docs.constructor.com/reference/browse-browse-results for details
* @param {object} [userParameters] - Parameters relevant to the user request
Expand Down Expand Up @@ -351,6 +358,7 @@ class Browse {
* @param {string[]} [parameters.hiddenFields] - Hidden metadata fields to return
* @param {string[]} [parameters.hiddenFacets] - Hidden facet fields to return
* @param {object} [parameters.variationsMap] - The variations map object to aggregate variations. Please refer to https://docs.constructor.com/reference/shared-variations-mapping for details
* @param {object} [parameters.filterMatchTypes] - An object specifying whether results must match `all`, `any` or `none` of a given filter
* @param {object} [userParameters] - Parameters relevant to the user request
* @param {number} [userParameters.sessionId] - Session ID, utilized to personalize results
* @param {string} [userParameters.clientId] - Client ID, utilized to personalize results
Expand Down
7 changes: 7 additions & 0 deletions src/modules/search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ function createSearchUrl(query, parameters, userParameters, options, isVoiceSear
variationsMap,
preFilterExpression,
qsParam,
filterMatchTypes,
} = parameters;

// Pull page from parameters
Expand All @@ -90,6 +91,11 @@ function createSearchUrl(query, parameters, userParameters, options, isVoiceSear
queryParams.filters = filters;
}

// Pull filter match types from parameters
if (filterMatchTypes) {
queryParams.filter_match_types = filterMatchTypes;
}

// Pull sort by from parameters
if (sortBy) {
queryParams.sort_by = sortBy;
Expand Down Expand Up @@ -184,6 +190,7 @@ class Search {
* @param {string[]} [parameters.hiddenFields] - Hidden metadata fields to return
* @param {string[]} [parameters.hiddenFacets] - Hidden facet fields to return
* @param {object} [parameters.variationsMap] - The variations map object to aggregate variations. Please refer to https://docs.constructor.com/reference/shared-variations-mapping for details
* @param {object} [parameters.filterMatchTypes] - An object specifying whether results must match `all`, `any` or `none` of a given filter
* @param {object} [parameters.preFilterExpression] - Faceting expression to scope search results. Please refer to https://docs.constructor.com/reference/configuration-collections for details
* @param {object} [parameters.qsParam] - Parameters listed above can be serialized into a JSON object and parsed through this parameter. Please refer to https://docs.constructor.com/reference/search-search-resultsqueries for details
* @param {object} [userParameters] - Parameters relevant to the user request
Expand Down
21 changes: 12 additions & 9 deletions src/types/browse.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export interface BrowseParameters {
hiddenFacets?: string[];
variationsMap?: VariationsMap;
qsParam?: Record<string, any>;
filterMatchTypes?: Record<string, 'all' | 'any' | 'none'>;
}

declare class Browse {
Expand Down Expand Up @@ -78,9 +79,9 @@ declare class Browse {

/* Browse results returned from server */
interface BrowseResponse<ResponseType> extends Record<string, any> {
request?: Partial<BrowseRequestType>;
response?: Partial<ResponseType>;
result_id?: string;
request: Partial<BrowseRequestType>;
response: Partial<ResponseType>;
result_id: string;
ad_based?: boolean;
}

Expand Down Expand Up @@ -110,7 +111,9 @@ export interface GetBrowseResultsResponseData extends Record<string, any> {
refined_content: Record<string, any>[];
total_num_results: number;
features: Partial<Feature>[];
collection: Partial<Collection>;
collection?: Partial<Collection>;
related_searches?: Record<string, any>[];
related_browse_pages?: Record<string, any>[];
}

export interface BrowseResultData extends Record<string, any> {
Expand All @@ -122,23 +125,23 @@ export interface BrowseResultData extends Record<string, any> {
value: string;
is_slotted: false;
labels: Record<string, any>;
variations: Record<string, any>[];
variations_map: Record<string, any> | Record<string, any>[];
variations?: Record<string, any>[];
variations_map?: Record<string, any> | Record<string, any>[];
}

export interface BrowseRequestType extends Record<string, any> {
browse_filter_name: string;
browse_filter_value: string;
filter_match_types: Record<string, any>;
filters: Record<string, any>;
filter_match_types?: Record<string, any>;

@constructor-claude-bedrock constructor-claude-bedrock Bot Oct 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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'>;

filters?: Record<string, any>;
fmt_options: Record<string, any>;
num_results_per_page: number;
page: number;
section: string;
sort_by: string;
sort_order: string;
term: string;
query: string;
query?: string;
features: Partial<RequestFeature>;
feature_variants: Partial<RequestFeatureVariant>;
searchandized_items: Record<string, any>;
Expand Down
44 changes: 40 additions & 4 deletions src/types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ export * from './tasks';
export * from './tracker';
export * from './searchandising';

type RequireAtLeastOne<T, Keys extends keyof T = keyof T> =

@constructor-claude-bedrock constructor-claude-bedrock Bot Oct 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Pick<T, Exclude<keyof T, Keys>> &
{ [K in Keys]-?: Required<Pick<T, K>> & Partial<Pick<T, Exclude<Keys, K>>> }[Keys];

export interface NetworkParameters extends Record<string, any> {
timeout?: number;
}
Expand Down Expand Up @@ -148,16 +152,48 @@ export interface Variation extends Record<string, any> {
data?: ItemData;
}

export interface VariationsMapSingleFilter {
field: string;
value: string | number | boolean;
}

export interface VariationsMapRange {
field: string;
range: FilterExpressionRangeValue;
}

export type FilterNode = VariationsMapSingleFilter | VariationsMapRange;

export type FilterBy = RequireAtLeastOne<{
and?: Array<FilterNode | FilterBy>;
or?: Array<FilterNode | FilterBy>;
not?: FilterNode | FilterBy;
}>;

export type Aggregation = 'first' | 'min' | 'max' | 'all' | 'count' | 'field_count' | 'value_count';

export interface VariationsMapBaseValue {
aggregation: Aggregation;
field: string;
}

export interface VariationsMapValueCount extends VariationsMapBaseValue {
aggregation: 'value_count';
value: boolean | number | string;
}

export interface VariationsMapStandardValue extends VariationsMapBaseValue {
aggregation: Exclude<Aggregation, 'value_count'>;
}

export interface VariationsMap {
group_by: Array<{
name: string,
field: string
}>;
filter_by?: FilterBy;
values: {
[key: string]: {
aggregation: 'first' | 'min' | 'max' | 'all',
field: string
},
[key: string]: VariationsMapValueCount | VariationsMapStandardValue,
},
dtype: 'array' | 'object'
}
Expand Down
3 changes: 3 additions & 0 deletions src/types/search.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export interface SearchParameters {
hiddenFacets?: string[];
variationsMap?: VariationsMap;
qsParam?: Record<string, any>;
filterMatchTypes?: Record<string, 'all' | 'any' | 'none'>;
}

declare class Search {
Expand Down Expand Up @@ -68,6 +69,8 @@ export interface Response extends Record<string, any> {
refined_content: Record<string, any>[];
total_num_results: number;
features: Partial<Feature>[];
related_searches?: Record<string, any>[];
related_browse_pages?: Record<string, any>[];

@constructor-claude-bedrock constructor-claude-bedrock Bot Oct 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 interface SearchRequestType extends Record<string, any> {
Expand Down
39 changes: 37 additions & 2 deletions src/types/tests/browse.test-d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import { expectAssignable } from 'tsd';
import { GetBrowseResultsResponse } from '../browse';
import { expectAssignable, expectNotAssignable } from 'tsd';
import {
BrowseParameters,
BrowseResultData,
GetBrowseResultsResponse,
} from '../browse';

expectAssignable<GetBrowseResultsResponse>({
response: {
Expand Down Expand Up @@ -118,3 +122,34 @@ expectAssignable<GetBrowseResultsResponse>({
},
ad_based: true,
});

expectAssignable<BrowseParameters>({
filters: { size: 'medium' },
filterMatchTypes: { size: 'all' },
});

expectNotAssignable<BrowseParameters>({
filterMatchTypes: { size: 'some' },
});

expectNotAssignable<GetBrowseResultsResponse>({

@constructor-claude-bedrock constructor-claude-bedrock Bot Oct 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

request: {},
response: { results: [] },
});

expectAssignable<GetBrowseResultsResponse>({
request: {},
response: {
related_searches: [{ term: 'dog toys' }],
related_browse_pages: [{ filter_name: 'group_id', filter_value: 'toys' }],
},
result_id: 'e5941e13-f4ca-4efb-9326-893fd49b4e71',
});

expectAssignable<BrowseResultData>({
matched_terms: [],
data: { id: '123' },
value: 'Name',
is_slotted: false,
labels: {},
});
36 changes: 34 additions & 2 deletions src/types/tests/types.test-d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expectAssignable } from 'tsd';
import { FilterExpression } from '../index';
import { expectAssignable, expectNotAssignable } from 'tsd';
import { FilterExpression, VariationsMap } from '../index';

expectAssignable<FilterExpression>({
or: [
Expand Down Expand Up @@ -31,3 +31,35 @@ expectAssignable<FilterExpression>({
},
],
});

expectAssignable<VariationsMap>({
group_by: [{ name: 'variation', field: 'data.variation_id' }],
filter_by: {
and: [
{ field: 'data.brand', value: 'Best' },
{ not: { field: 'data.price', range: [100, 'inf'] } },
],
},
values: {
min_price: { aggregation: 'min', field: 'data.price' },
total: { aggregation: 'count', field: 'data.variation_id' },
sizes: { aggregation: 'field_count', field: 'data.size' },
in_stock: { aggregation: 'value_count', field: 'data.in_stock', value: true },
},
dtype: 'object',
});

expectNotAssignable<VariationsMap>({
group_by: [{ name: 'variation', field: 'data.variation_id' }],
values: {
in_stock: { aggregation: 'value_count', field: 'data.in_stock' },
},
dtype: 'object',
});

expectNotAssignable<VariationsMap>({
group_by: [{ name: 'variation', field: 'data.variation_id' }],
filter_by: {},
values: {},
dtype: 'array',
});
Loading