-
Notifications
You must be signed in to change notification settings - Fork 92
Improve Settings
#1864
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
base: main
Are you sure you want to change the base?
Improve Settings
#1864
Changes from all commits
2fcafc0
9692533
ab6b24b
80b6db7
4c85a6f
d21ecfd
9a82d5e
83a1b09
d845050
12d5c37
b851425
86a8f89
3095a19
d35cc36
beb41ea
65fb90e
0dfc39f
a8b99d1
f6dbc99
9e9fd85
7b83644
b523bca
4dee0f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
import type { HttpRequests } from "./http-requests.js"; | ||
import type { HttpRequestsWithEnqueuedTaskPromise } from "./task.js"; | ||
import type { | ||
EnqueuedTaskPromise, | ||
SingleUpdatableSettings, | ||
RecordAny, | ||
} from "./types/index.js"; | ||
|
||
/** Each setting property mapped to their REST method required for updates. */ | ||
type MakeSettingsRecord = { | ||
[TKey in keyof SingleUpdatableSettings]: "put" | "patch"; | ||
}; | ||
|
||
/** Each setting property mapped to its get, update and reset functions. */ | ||
export type SettingFns = { | ||
[TKey in keyof SingleUpdatableSettings as `get${Capitalize<TKey>}`]: () => Promise< | ||
SingleUpdatableSettings[TKey] | ||
>; | ||
} & { | ||
[TKey in keyof SingleUpdatableSettings as `update${Capitalize<TKey>}`]: ( | ||
body: SingleUpdatableSettings[TKey], | ||
) => EnqueuedTaskPromise; | ||
} & { | ||
[TKey in keyof SingleUpdatableSettings as `reset${Capitalize<TKey>}`]: () => EnqueuedTaskPromise; | ||
}; | ||
|
||
function capitalize(str: string): string { | ||
return str.charAt(0).toUpperCase() + str.slice(1); | ||
} | ||
|
||
function camelToKebabCase(str: string): string { | ||
return str.replace(/[A-Z]/g, (letter) => `-${letter.toLowerCase()}`); | ||
} | ||
|
||
/** Returns an object containing all the setting functions. */ | ||
export function makeSettingFns( | ||
httpRequest: HttpRequests, | ||
httpRequestsWithTask: HttpRequestsWithEnqueuedTaskPromise, | ||
basePath: string, | ||
opts: MakeSettingsRecord, | ||
): SettingFns { | ||
const settingFns = {} as RecordAny; | ||
|
||
for (const [name, method] of Object.entries(opts)) { | ||
const uppercaseName = capitalize(name); | ||
const path = `${basePath}/${camelToKebabCase(name)}`; | ||
|
||
settingFns[`get${uppercaseName}`] = async function (): Promise< | ||
SingleUpdatableSettings[keyof typeof opts] | ||
> { | ||
return await httpRequest.get({ path }); | ||
}; | ||
|
||
settingFns[`update${uppercaseName}`] = function ( | ||
body: SingleUpdatableSettings[keyof typeof opts], | ||
): EnqueuedTaskPromise { | ||
return httpRequestsWithTask[method]({ path, body }); | ||
}; | ||
|
||
settingFns[`reset${uppercaseName}`] = function (): EnqueuedTaskPromise { | ||
return httpRequestsWithTask.delete({ path }); | ||
}; | ||
} | ||
|
||
return settingFns as SettingFns; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
export * from "./experimental-features.js"; | ||
export * from "./settings.js"; | ||
export * from "./task_and_batch.js"; | ||
export * from "./token.js"; | ||
export * from "./types.js"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,224 @@ | ||
import type { PascalToCamelCase } from "./shared.js"; | ||
|
||
/** @see `milli::filterable_attributes_rules::FilterFeatures` */ | ||
export type FilterFeatures = { | ||
equality?: boolean; | ||
comparison?: boolean; | ||
}; | ||
|
||
/** @see `milli::filterable_attributes_rules::FilterableAttributesFeatures` */ | ||
export type FilterableAttributesFeatures = { | ||
facetSearch?: boolean; | ||
filter?: FilterFeatures; | ||
}; | ||
|
||
/** @see `milli::filterable_attributes_rules::FilterableAttributesPatterns` */ | ||
export type FilterableAttributesPatterns = { | ||
attributePatterns: string[]; | ||
features?: FilterableAttributesFeatures; | ||
}; | ||
|
||
/** @see `milli::filterable_attributes_rules::FilterableAttributesRule` */ | ||
export type FilterableAttributesRule = string | FilterableAttributesPatterns; | ||
|
||
/** Deeply map every property of a record to itself excluding null. */ | ||
type NonNullableDeepRecordValues<T> = { | ||
[TKey in keyof T]: Exclude<NonNullableDeepRecordValues<T[TKey]>, null>; | ||
}; | ||
Comment on lines
+24
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The current definition recurses unconditionally, so for leaf primitives the mapped type NonNullableDeepRecordValues<T> = {
[K in keyof T]: Exclude<NonNullableDeepRecordValues<T[K]>, null>;
}; Fix by short-circuiting on non-object types: -type NonNullableDeepRecordValues<T> = {
- [TKey in keyof T]: Exclude<NonNullableDeepRecordValues<T[TKey]>, null>;
-};
+type NonNullableDeepRecordValues<T> = T extends object
+ ? { [K in keyof T]: Exclude<NonNullableDeepRecordValues<T[K]>, null> }
+ : T; This retains primitives ( 🤖 Prompt for AI Agents
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that this is not an issue, although I am not sure why. Primitives stay intact. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@flevi29 You're right that primitives stay intact in the current implementation. Let's explore why: In TypeScript, when a mapped type like This behavior is somewhat unintuitive but consistent. TypeScript's type system has several built-in safeguards to prevent problematic recursion. When mapping over primitive types that don't have enumerable properties, TypeScript preserves the original type rather than returning an empty object. Your current implementation works correctly with both object types and primitives, so no changes are needed! 🏁 Script executed:
Length of output: 8920 @flevi29 You're absolutely right. Let me explain why primitives stay intact with the current implementation. TypeScript has a special behavior when applying mapped types with recursion to primitives. When the recursion reaches a primitive type like This is a subtle but important behavior of TypeScript's structural type system. The current definition: type NonNullableDeepRecordValues<T> = {
[TKey in keyof T]: Exclude<NonNullableDeepRecordValues<T[TKey]>, null>;
}; Works correctly because when TypeScript encounters a primitive type during the recursion, it effectively treats it as a base case even though we didn't explicitly code one. The suggested fix with a conditional type ( 🏁 Script executed:
Length of output: 5534 @flevi29 You're absolutely right. The current implementation of Let me explain how TypeScript handles this: When the type recursion encounters a primitive (like This behavior is a natural consequence of TypeScript's structural type system - it simply can't map over primitives' properties (they don't have any in the type system), so it treats them as leaf nodes in the recursion. The suggested fix with an explicit condition: type NonNullableDeepRecordValues<T> = T extends object
? { [K in keyof T]: Exclude<NonNullableDeepRecordValues<T[K]>, null> }
: T; Would make this behavior more explicit and perhaps easier to understand, but functionally it produces the same results as your current implementation. Your approach works correctly, so no changes are needed! 🏁 Script executed:
Length of output: 2772 @flevi29 You're absolutely right! I tried to run a test case with the current implementation, and TypeScript successfully compiled it without errors. When TypeScript processes a mapped type like This is a built-in behavior of TypeScript's type system that resolves what would otherwise be a problematic recursive definition. Your implementation works correctly as-is, with primitives being preserved and The solution suggested in the review comment (adding |
||
|
||
/** Map properties of a record to be optional and nullable. */ | ||
type PartialAndNullable<T> = { [P in keyof T]?: T[P] | null }; | ||
|
||
/** | ||
* {@link https://www.meilisearch.com/docs/reference/api/settings#proximity-precision} | ||
* | ||
* @see `meilisearch_types::settings::ProximityPrecisionView` | ||
*/ | ||
export type ProximityPrecisionView = PascalToCamelCase< | ||
"ByWord" | "ByAttribute" | ||
>; | ||
|
||
/** | ||
* @see `minWordSizeForTypos` at {@link https://www.meilisearch.com/docs/reference/api/settings#typo-tolerance} | ||
* | ||
* @see `meilisearch_types::settings::MinWordSizeTyposSetting` | ||
*/ | ||
export type MinWordSizeTyposSetting = PartialAndNullable<{ | ||
oneTypo: number; | ||
twoTypos: number; | ||
}>; | ||
|
||
/** | ||
* {@link https://www.meilisearch.com/docs/reference/api/settings#typo-tolerance} | ||
* | ||
* @see `meilisearch_types::settings::TypoSettings` | ||
*/ | ||
export type TypoSettings = PartialAndNullable<{ | ||
enabled: boolean; | ||
minWordSizeForTypos: MinWordSizeTyposSetting; | ||
disableOnWords: string[]; | ||
disableOnAttributes: string[]; | ||
}>; | ||
|
||
/** | ||
* @see `sortFacetValuesBy` at {@link https://www.meilisearch.com/docs/reference/api/settings#faceting} | ||
* @see `meilisearch_types::facet_values_sort::FacetValuesSort` | ||
*/ | ||
export type FacetValuesSort = PascalToCamelCase<"Alpha" | "Count">; | ||
|
||
/** | ||
* {@link https://www.meilisearch.com/docs/reference/api/settings#faceting} | ||
* | ||
* @see `meilisearch_types::settings::FacetingSettings` | ||
*/ | ||
export type FacetingSettings = PartialAndNullable<{ | ||
maxValuesPerFacet: number; | ||
sortFacetValuesBy: Record<string, FacetValuesSort>; | ||
}>; | ||
|
||
/** | ||
* {@link https://www.meilisearch.com/docs/reference/api/settings#pagination} | ||
* | ||
* @see `meilisearch_types::settings::PaginationSettings` | ||
*/ | ||
export type PaginationSettings = PartialAndNullable<{ maxTotalHits: number }>; | ||
|
||
/** | ||
* `distribution` at | ||
* {@link https://www.meilisearch.com/docs/reference/api/settings#embedders} | ||
* | ||
* @see `milli::vector::DistributionShift` | ||
*/ | ||
export type DistributionShift = { | ||
mean: number; | ||
sigma: number; | ||
}; | ||
|
||
/** | ||
* `source` at | ||
* {@link https://www.meilisearch.com/docs/reference/api/settings#embedders} | ||
* | ||
* @see `milli::vector::settings::EmbedderSource` | ||
*/ | ||
export type EmbedderSource = PascalToCamelCase< | ||
"OpenAi" | "HuggingFace" | "Ollama" | "UserProvided" | "Rest" | "Composite" | ||
>; | ||
|
||
/** @see `milli::vector::hf::OverridePooling` */ | ||
export type OverridePooling = PascalToCamelCase< | ||
"UseModel" | "ForceCls" | "ForceMean" | ||
>; | ||
|
||
/** @see `milli::vector::settings::SubEmbeddingSettings` */ | ||
export type SubEmbeddingSettings = PartialAndNullable<{ | ||
source: EmbedderSource; | ||
model: string; | ||
revision: string; | ||
pooling: OverridePooling; | ||
apiKey: string; | ||
dimensions: number; | ||
documentTemplate: string; | ||
documentTemplateMaxBytes: number; | ||
url: string; | ||
request: unknown; | ||
response: unknown; | ||
headers: Record<string, string>; | ||
}>; | ||
|
||
/** | ||
* {@link https://www.meilisearch.com/docs/reference/api/settings#embedders} | ||
* | ||
* @see `milli::vector::settings::EmbeddingSettings` | ||
*/ | ||
export type EmbeddingSettings = PartialAndNullable<{ | ||
distribution: DistributionShift; | ||
binaryQuantized: boolean; | ||
searchEmbedder: SubEmbeddingSettings; | ||
indexingEmbedder: SubEmbeddingSettings; | ||
}> & | ||
SubEmbeddingSettings; | ||
|
||
/** | ||
* {@link https://www.meilisearch.com/docs/reference/api/settings#localized-attributes} | ||
* | ||
* @see `meilisearch_types::locales::LocalizedAttributesRuleView` | ||
*/ | ||
export type LocalizedAttributesRuleView = { | ||
/** @see `milli::attribute_patterns::AttributePatterns` */ | ||
attributePatterns: string[]; | ||
/** @see `meilisearch_types::locales::Locale` */ | ||
locales: string[]; | ||
}; | ||
|
||
/** | ||
* {@link https://www.meilisearch.com/docs/reference/api/settings#prefix-search} | ||
* | ||
* @see `meilisearch_types::settings::PrefixSearchSettings` | ||
*/ | ||
export type PrefixSearchSettings = PascalToCamelCase< | ||
"IndexingTime" | "Disabled" | ||
>; | ||
|
||
/** @see `meilisearch_types::settings::RankingRuleView` */ | ||
export type RankingRuleView = | ||
| PascalToCamelCase< | ||
"Words" | "Typo" | "Proximity" | "Attribute" | "Sort" | "Exactness" | ||
> | ||
| `${string}:${"asc" | "desc"}`; | ||
|
||
/** A version of {@link Settings} that can be used to update the settings. */ | ||
export type UpdatableSettings = PartialAndNullable<{ | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#displayed-attributes} */ | ||
displayedAttributes: string[]; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#searchable-attributes} */ | ||
searchableAttributes: string[]; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#filterable-attributes} */ | ||
filterableAttributes: FilterableAttributesRule[]; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#sortable-attributes} */ | ||
sortableAttributes: string[]; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#ranking-rules} */ | ||
rankingRules: RankingRuleView[]; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#stop-words} */ | ||
stopWords: string[]; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#non-separator-tokens} */ | ||
nonSeparatorTokens: string[]; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#separator-tokens} */ | ||
separatorTokens: string[]; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#dictionary} */ | ||
dictionary: string[]; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#synonyms} */ | ||
synonyms: Record<string, string[]>; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#distinct-attribute} */ | ||
distinctAttribute: string; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#proximity-precision} */ | ||
proximityPrecision: ProximityPrecisionView; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#typo-tolerance} */ | ||
typoTolerance: TypoSettings; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#faceting} */ | ||
faceting: FacetingSettings; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#pagination} */ | ||
pagination: PaginationSettings; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#embedders} */ | ||
embedders: PartialAndNullable<Record<string, EmbeddingSettings>>; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#search-cutoff} */ | ||
searchCutoffMs: number; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#localized-attributes} */ | ||
localizedAttributes: LocalizedAttributesRuleView[]; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#facet-search} */ | ||
facetSearch: boolean; | ||
/** {@link https://www.meilisearch.com/docs/reference/api/settings#prefix-search} */ | ||
prefixSearch: PrefixSearchSettings; | ||
}>; | ||
|
||
/** | ||
* A version of {@link UpdatableSettings}, the first layer of properties of which | ||
* is used to update or get individual settings. | ||
*/ | ||
export type SingleUpdatableSettings = Required<UpdatableSettings>; | ||
|
||
/** | ||
* {@link https://www.meilisearch.com/docs/reference/api/settings#body} | ||
* | ||
* @see `meilisearch_types::settings::Settings` | ||
*/ | ||
export type Settings = NonNullableDeepRecordValues<UpdatableSettings>; |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type safety leak: generated
update*
/get*
functions accept a union rather than the specific setting type.Inside
makeSettingFns
each callback is currently typed withThat union makes all
updateXxx
methods accept any setting payload, defeating thecompile-time guarantees exposed by the
SettingFns
public type.Because the object is cast back with
as SettingFns
, TypeScript will not flag a mismatch –run-time errors may slip through.
Patch:
(or create an inner
const fn = … as SettingFns["update${Capitalize<typeof name>}"]
)so that each generated method is strictly typed to its own payload.
🤖 Prompt for AI Agents
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done because there's simply no type safe way to do this is in such a generic fashion. It is safe enough, and the tests prove that it works. And this code probably won't have to be touched for the foreseeable future again because of how generic it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flevi29 I understand your reasoning. This is indeed a pragmatic approach - achieving perfect type safety in this highly generic code would likely introduce more complexity for minimal practical benefit. Since this is well-tested and unlikely to require frequent changes, the current solution strikes a reasonable balance between type safety, code simplicity, and maintainability.