algoliasearch-client-javascript

algoliasearch-client-javascript copied to clipboard

The problem

Currently, most of the types (if not all?) are marked with readonly all over the place. In some types it totally makes sense, though in some types it's just super inconvenient to have the property marked as readonly. One example is the SearchOptions type, which is used in things like SearchIndex's search method for the requestOptions argument:

export declare type SearchIndex = SearchIndex_2 & {
    readonly search: <TObject>(query: string, requestOptions?: RequestOptions & SearchOptions) => Readonly<Promise<SearchResponse<TObject>>>;
    readonly searchForFacetValues: (facetName: string, facetQuery: string, requestOptions?: RequestOptions & SearchOptions) => Readonly<Promise<SearchForFacetValuesResponse>>;
    readonly findAnswers: <TObject>(query: string, queryLanguages: readonly string[], requestOptions?: RequestOptions & FindAnswersOptions) => Readonly<Promise<FindAnswersResponse<TObject>>>;
};

Now imagine having an application, that conditionally needs to add more properties to the requestOptions argument, depending on some internal application logic, which could look roughly like this:

import {RequestOptions} from '@algolia/transporter';
import {SearchOptions} from "@algolia/client-search";

let requestOptions: RequestOptions & SearchOptions = {
    facets,
    filters,
    page,
    hitsPerPage,
};

if (someConditions)
{
    requestOptions.aroundLatLng = "some, value;

    if (someOtherConditions)
    {
        requestOptions.aroundRadius = "someValue";
    }
}

index.current.search(query, requestOptions);

This is currently strictly forbidden as SearchOptions explicitly says that the property is readonly, thus it cannot be assigned after its initial creation.

Now I have a couple of options, which all are equally a hack and come with potential side effects, that I'm not very fond of:

Option 1: Remove the readonly modifier within my app code

We could simply remove the readonly modifier from the type like so:

import {RequestOptions} from '@algolia/transporter';
import {SearchOptions} from "@algolia/client-search";

type Writeable<T> = { -readonly [P in keyof T]: T[P] };

let requestOptions: RequestOptions & Writeable<SearchOptions> = {
    facets,
    filters,
    page,
    hitsPerPage,
};

if (someConditions)
{
    requestOptions.aroundLatLng = "some, value;

    if (someOtherConditions)
    {
        requestOptions.aroundRadius = "someValue";
    }
}

index.current.search(query, requestOptions);

The potential downside is that we'd make it appear that some property, which really should be readonly, not readonly and can be assigned. It's also not very forward stable.

Option 2: Don't use the Algolia types

import {RequestOptions} from '@algolia/transporter';
import {SearchOptions} from "@algolia/client-search";

let requestOptions: Record<string, string|string[]|boolean|number|undefined> = {
    facets,
    filters,
    page,
    hitsPerPage,
};

if (someConditions)
{
    requestOptions.aroundLatLng = "some, value;

    if (someOtherConditions)
    {
        requestOptions.aroundRadius = "someValue";
    }
}

index.current.search(query, requestOptions);

It sucks for multiple reasons: I'm not getting any auto-completion, real type and property checking and it's not forward stable.

Option 3: Remove the readonly from the type itself and use Readonly<SearchOptions> where necessary within Algolia

import {RequestOptions} from '@algolia/transporter';
import {SearchOptions} from "@algolia/client-search";

let requestOptions: RequestOptions & SearchOptions = {
    facets,
    filters,
    page,
    hitsPerPage,
};

if (someConditions)
{
    requestOptions.aroundLatLng = "some, value;

    if (someOtherConditions)
    {
        requestOptions.aroundRadius = "someValue";
    }
}

index.current.search(query, requestOptions);

The updated type definitions for SearchIndex would look like this:

export declare type SearchIndex = SearchIndex_2 & {
    readonly search: <TObject>(query: string, requestOptions?: Readonly<RequestOptions & SearchOptions>) => Readonly<Promise<SearchResponse<TObject>>>;
    readonly searchForFacetValues: (facetName: string, facetQuery: string, requestOptions?: Readonly<RequestOptions & SearchOptions>) => Readonly<Promise<SearchForFacetValuesResponse>>;
    readonly findAnswers: <TObject>(query: string, queryLanguages: readonly string[], requestOptions?: Readonly<RequestOptions & FindAnswersOptions>) => Readonly<Promise<FindAnswersResponse<TObject>>>;
};

Conclusion/Proposal

By far the best option is number 3 because I as a library consumer will get all of TypeScript's benefits and the only downside would be the amount of work necessary to refactor internal (and potentially external) APIs to use Readonly<SearchOptions> where really necessary. This moves the readonly-check away from the assignment into the function and does exactly what you want: Not accidentally modify your input arguments.

A slightly dumbed down example can be seen here:

• Current version with readonly properties: https://www.typescriptlang.org/play?#code/C4TwDgpgBAyhCGAnAxgCwPJmASwPYDsBnKAXigG8AoKGqRBAEwIBsQp4B+ALikOEWz4A5gG5qtevCb5WUAEbde-QaMoBfMZQBmAV3zIcBKFty45SKAAoG8YPB5wkaTIaIBKKDwBuubA0pUtFA2dgB08KRQAETwUWJBIfChcpFRcnHqlJTMEMDGpg4IKBhYeESRgbT20Sa4UQA0mZTYWlb8OhBuAeI0tcmp5ogZalm1g5a1biJAA
• Proposed version with Readonly<T> arguments: https://www.typescriptlang.org/play?#code/C4TwDgpgBAyhCGAnAxgCwPJmASwPYDsBnKAXigG8AoKGqeAfgC4pDhFt8BzAbmtoCMmLNhx6UAvr0oAzAK75kOAlGm5c-JFAAUAE3jB4zAEoIdBADYgAPHCRpMSogD4AlFGYA3XNh2UqtKD0DADp4UigAIngI3gCg+GD+cIj+GIlKSnMIYBU1ZlsUDCw8InD-WkNI1VwIgBp0ymxpbTZZCBc-PhpqxOSNRDTxDOr+rWqXbiA

What do you guys think?

/cc @Haroenv

hey, thanks for the very thorough proposal. I can only agree, it has been a bit complicated to handle those readonly params even readonly. They make sense and are perfectly logical, but too restrictive.

We are planning a next major for this client, and those types will most likely be gone. (No ETA) In the meantime we could remove them but that would requires a proper major too to avoid issues with other users.

Is there anything I can help with speeding up this process? Or is this part of a major overhaul as part of a new major release?

I actually think removing readonly might not be a major version, as it's less restrictive: https://www.typescriptlang.org/play?#code/C4TwDgpgBAhgjFAvFA3gWAFBW1AThGAEwHsA7AGxCgCMAuKAZ2FwEtSBzTAX001ElgAmJKkw4a9Jqw7deGAGYBXUgGNgLMlHlwAFDFzt68AJSoeC5Wo2ktgvQaODTKc5m06UdKAHJvXY252nvS+-nLuwT5+sAxQKmRMAQpBXqExcQnASZjxpEywBnBGCMiRoeG6+uxwSfJ2VTVyuflVgo4iZX4V9uxOgT1OQA

Not sure if I'm missing anything though, maybe you can't cast or assume something is readonly anymore?

I'm not sure either, though I'd believe that it'd only really affect Algolia's internals if some new method code hasn't been updated to Readonly<T> and it's really trying to mutate the input params. Existing internal and external calling code shouldn't break with this change as we're not suddenly trying to mutate everything everywhere. Only newly written code should be affected if it doesn't rely on Readonly<T>, which might be the only good reason against putting this change into a minor version as documentation and internal training would take place first since all engineers need to be made aware of this potentially dangerous change first, as they can no longer rely on getting automatic readonly on each input param, as they'd have to explicitly opt-in first.

At the end of the day I'm fine with either option: Major or minor version. That's totally up to you :)

I'd like to know if there is any progress on this issue?

I have to agree with @keichinger this is really cumbersome in many scenarios.

Hey @unwobbling, readonly have been removed in v5 (https://github.com/algolia/algoliasearch-client-javascript/tree/next), you can track development on https://github.com/algolia/api-clients-automation