algolia / algoliasearch-client-javascript

⚡️ A fully-featured and blazing-fast JavaScript API client to interact with Algolia.
https://www.algolia.com/doc/api-client/javascript/getting-started/
MIT License
1.33k stars 222 forks source link

No clear migration guide for v5 and no explicit typing #1537

Closed WavyWalk closed 1 month ago

WavyWalk commented 2 months ago

We want to migrate to v4 from v5, but migrating even this snippet is problematic:

const object = await algoliasearch(algoliaApplicationId, algoliaApiKey)
      .initIndex(productsAlgoliaIndex)
      .getObjects<AlgoliaRecord>(productIds)

I see in source code that getObjects exist in v5 but one can't pass index and it's not in docs, and there's no index in types.

Also types are not straightforward (I get that it's generated but still), so rules like @typescript-eslint/no-unsafe-assignment fail because of any's in unions.

As well not sure how to proceed with now failing type checks for react instantsearch because things like: hitsPerPage, analyticsTags on <Configure />reference the types of v5 which do not have it.

Do you recommend to migrate now or wait till there will be a proper migration guide and types will be improved?

OutdatedGuy commented 2 months ago

You need to update your code using this documentation. It doesn't feel complete so it sucks. You might have to view code documentation/intellisense from your code editor/IDE.

Before:

import algoliasearch from "algoliasearch";

const object = await algoliasearch(algoliaApplicationId, algoliaApiKey)
  .initIndex(productsAlgoliaIndex)
  .getObjects<AlgoliaRecord>(productIds);

After:

import { algoliasearch } from "algoliasearch";

const object = await algoliasearch(algoliaApplicationId, algoliaApiKey)
  .getObjects<AlgoliaRecord>({
    requests: [
      {
        indexName: productsAlgoliaIndex,
        objectID: productId1,
      },
      {
        indexName: productsAlgoliaIndex,
        objectID: productId2,
      },
      // ...
    ],
  });
shortcuts commented 2 months ago

Hey @WavyWalk thanks for using the v5 client already, and sorry for the lack of documentation, we are currently working on providing snippets and guides! (and thanks @OutdatedGuy for the help :D)

I see in source code that getObjects exist in v5 but one can't pass index and it's not in docs, and there's no index in types.

The initIndex of the previous version was kind of magical syntactic sugar (and not suited for generated clients too), as mentioned here, every methods are now available at the root of the client, if you were using one via initIndex before, it will now expect an indexName parameter if it's a single-index method, or an indexName in the requests if it's a multi-index method

In the meantime of the guides being available, you can browse our API reference which includes a code snippet for every methods of the client, for every languages.

As well not sure how to proceed with now failing type checks for react instantsearch because things like: hitsPerPage, analyticsTags on reference the types of v5 which do not have it.

I'll investigate this but since I'm not super familiar with RIS cc @Haroenv

Do you recommend to migrate now or wait till there will be a proper migration guide and types will be improved?

Did you find any wrong typings or something that would prevent you from migrating? If so, I can fix those and help you move forward :) Otherwise I think it would be mostly migrating method signatures which (I hope) shouldn't be too different

Haroenv commented 2 months ago

for React InstantSearch, ensure you're on the very latest version, if you're on an older version this may not be inferred right. In my testing it also worked completely as expected, but maybe we missed a case? please make a full reproduction if anything isn't inferred as expected

WavyWalk commented 2 months ago

Here's repo with reproduction of the issues I mentioned: https://github.com/WavyWalk/reproduce-algolia-v5

all you can see in https://github.com/WavyWalk/reproduce-algolia-v5/blob/master/src/App.tsx

Haroenv commented 2 months ago

Oh no, you're right! I'm just realising that a last fix for the types was only done in https://github.com/algolia/instantsearch/pull/6270 which isn't merged yet. I'll get right on making sure CI passes there (unrelated failure) and we can release a fix soon

WavyWalk commented 2 months ago

regarding getObjects:

I see that objects can be fetched from multiple indexes and hence it accept array of index objectID pairs,

but if I pass hundreds of pairs with same index will it still make one call?

.getObjects<AlgoliaRecord>({
    requests: [
      {
        indexName: productsAlgoliaIndex,
        objectID: productId1,
      },
      {
        indexName: productsAlgoliaIndex,
        objectID: productId2,
      },
      // ...
    ],

more natural would be, does it support it?:


requests: [
      {
        indexName: someIndex,
        objectIds: Array<string>,
      },
      {
        indexName: otherIndex,
        objectID: Array<string>,
      }

```]
shortcuts commented 2 months ago

Hey

regarding getObjects:

I see that objects can be fetched from multiple indexes and hence it accept array of index objectID pairs,

but if I pass hundreds of pairs with same index will it still make one call?

Yes

more natural would be, does it support it?:

Those new clients are meant to be closer to the API, so this signatures is exactly what is expected from the REST endpoint

MattIPv4 commented 2 months ago

👋 Not to pile on here, but please could an actual migration guide be published here, that documents the actual signature changes for methods?

For example, it turns out browseObjects now takes an aggregator method, not batch, and the data passed to that method is of a different shape to before (an object containing a hits array, rather than just an array). Or take setSettings which now takes the settings as a nested indexSettings method and forwardToReplicas as part of the first object, not a second object. I had to inspect the source to find these things, and I should not have to do that when a migration guide supposedly exists...

Even the bits that are documented currently aren't great... wait was removed on methods and the replacements in the guide link out to documentation for the Python library. Even if they did link to the JS docs, it doesn't appear to be a 1:1 mapping of the methods -- if I call saveObjects I don't just get back taskID or an object containing it, instead I get back an array of objects containing task IDs... and so I need to manually call waitForTask for each one? This seems like a huge step back for DX...

I would complain far less if the documentation actually existed for the methods, but that doesn't exist either (sans a select few methods classed as "helpers" or unless you rely on code samples in the REST API docs which seem to be missing a bunch of options [like forwardToReplicas mentioned earlier])... so between no migration guide and no actual documentation, trying to upgrade to this new major is incredibly painful at present.

Haroenv commented 2 months ago

The React InstantSearch issue is solved now, thanks for holding on. The teams responsible are talking about how to add more migration guides etc., but in the mean time don't feel pressured to update if it's too complicated or unclear for you, v4 will continue to work for a long time (v3 even still works now).

Tirke commented 2 months ago

Hello 👋

Been trying to move to the v5 client like that

const { results } = await this.client.search<AlgoliaSearchResult>({ requests: [ { indexName: 'name', query: searchString, }, ], })

The provided results is an array of one object

{
  results: [
    {
      hits: [
        [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...], [Object ...]
      ],
      nbHits: 148,
      page: 0,
      nbPages: 3,
      hitsPerPage: 60,
      exhaustiveNbHits: true,
      exhaustive: [Object ...],
      query: "...",
      params: "query=...",
      index: "....",
      processingTimeMS: 1,
      processingTimingsMS: [Object ...],
      serverTimeMS: 3,
    }
  ],
}

But the type of the returned object is not correctly resolved, it is lacking nbHits and other params ...

image
Haroenv commented 2 months ago

@Tirke you may be interested in https://github.com/algolia/algoliasearch-client-javascript/issues/1536

Tirke commented 2 months ago

@Haroenv The issue I'm showcasing is still present on 5.1.0 :(

Haroenv commented 2 months ago

@Tirke yes, in 5.1.0 the alias searchForHits was added, which is like search, but only for hits

Tirke commented 2 months ago

Ah yes I skimmed a bit fst over the issue, those helpers are not showcased in the docs I guess? Thanks for the pointer :)

lukebennett88 commented 2 months ago

I'm also not sure how to upgrade to v5. Here's the my old code:

import { type Hit } from '@algolia/client-search';
import { default as algoliasearch } from 'algoliasearch/lite';

const ALGOLIA_APP_ID = '';
const ALGOLIA_SEARCH_API_KEY = '';

type SearchOptions = {
    indexName: string;
    query: string;
    pageParam?: number;
    hitsPerPage: number;
};

export async function search<TData>({ indexName, query, pageParam, hitsPerPage }: SearchOptions): Promise<{
    hits: Hit<TData>[];
    nextPage: number | undefined;
}> {
    const client = algoliasearch(ALGOLIA_APP_ID, ALGOLIA_SEARCH_API_KEY);
    const index = client.initIndex(indexName);

    const { hits, page, nbPages } = await index.search<TData>(query, {
        page: pageParam ?? 0,
        hitsPerPage,
    });

    const nextPage = page + 1 < nbPages ? page + 1 : undefined;

    return { hits, nextPage };
}
shortcuts commented 2 months ago

Ah yes I skimmed a bit fst over the issue, those helpers are not showcased in the docs I guess? Thanks for the pointer :)

Not yet, we are working on generating them!

I'm also not sure how to upgrade to v5. Here's the my old code:

We will soon provide a documentation page with a code snippet for every methods, along with a mapping with method renames

For your specific code snippet, it would be

import { Hit } from '@algolia/client-search';
import { liteClient as algoliasearch } from 'algoliasearch/lite';

const ALGOLIA_APP_ID = '';
const ALGOLIA_SEARCH_API_KEY = '';

type SearchOptions = {
    indexName: string;
    query: string;
    pageParam?: number;
    hitsPerPage: number;
};

export async function search<TData>({ indexName, query, pageParam, hitsPerPage }: SearchOptions): Promise<{
    hits: Hit<TData>[];
    nextPage: number | undefined;
}> {
    const client = algoliasearch(ALGOLIA_APP_ID, ALGOLIA_SEARCH_API_KEY);

    const { hits, page, nbPages } = await client.searchSingleIndex<TData>({
                indexName,
                searchParams: {
                  query,
          page: pageParam ?? 0,
          hitsPerPage,
                },
    });

    const nextPage = page + 1 < nbPages ? page + 1 : undefined;

    return { hits, nextPage };
}

You can find some example here https://www.algolia.com/doc/rest-api/search/#tag/Search

lukebennett88 commented 2 months ago

Thanks @shortcuts but that doesn't seem to work for me. There doesn't seem to be a searchSingleIndex method on client. There are no types for it, but I tried it anyway and it's not there.

I did manage to get this to work, but as you can see I've had to cast to any for the result.

import { type Hit } from '@algolia/client-search';
import { liteClient as algoliasearch } from 'algoliasearch/lite';

const ALGOLIA_APP_ID = '';
const ALGOLIA_SEARCH_API_KEY = '';

type SearchOptions = {
    indexName: string;
    query: string;
    pageParam?: number;
    hitsPerPage: number;
};

export async function search<TData>({ indexName, query, pageParam, hitsPerPage }: SearchOptions): Promise<{
    hits: Hit<TData>[];
    nextPage: number | undefined;
}> {
    const client = algoliasearch(ALGOLIA_APP_ID, ALGOLIA_SEARCH_API_KEY);

    const { results } = await client.search<TData>({
        requests: [
            {
                indexName,
                query,
                page: pageParam,
                hitsPerPage,
            },
        ],
    });

    // TODO: fix types
    const result = results[0] as any;
    const { hits, nbPages, page } = result;
    const nextPage = page + 1 < nbPages ? page + 1 : undefined;

    return { hits, nextPage };
}

Not sure if there's a better way to do this.

shortcuts commented 2 months ago

Thanks @shortcuts but that doesn't seem to work for me.

Ah yes sorry I forgot there's only the multi-index search on the lite client

I did manage to get this to work, but as you can see I've had to cast to any for the result.

then you'd also be interested by https://github.com/algolia/algoliasearch-client-javascript/issues/1537#issuecomment-2302413180

simonmaass commented 2 months ago

I also just found out after some digging that the lite client doesnt have the "searchSingleIndex" method. Is there a reason for it?

shortcuts commented 2 months ago

I also just found out after some digging that the lite client doesnt have the "searchSingleIndex" method. Is there a reason for it?

Bundle size/keeping the lite client as minimal as possible, since you can do single index search with the multi-index method (search)

nfarina commented 2 months ago

I just finished migrating our own codebase from Algolia v4 to v5 and found the process very confusing. There's one page that I found with "upgrade" examples and it felt pretty anemic.

The main problem IMHO is that the (new?) TypeScript types are extremely convoluted and hard to follow. For example, when I Command+Click to see what the definition of searchSingleIndex is (because I've never heard of that method before), I get this type definition:

searchSingleIndex<T>({ indexName, searchParams }: import("../model").
SearchSingleIndexProps, requestOptions?: import("@algolia/client-common").RequestOptions):
Promise<import("../model").SearchResponse<T>>;

It looks auto-generated, no line breaks for clarity, no comments. I've never seen imports used as type parameters - probably because a human would never write this! I see this (buried in a soup of other typedefs/imports) and I want to give up.

Or when I try to figure out what options are available in SearchQuery which is the main type for interacting with Algolia. Command+Click, and I get:

export type SearchQuery = SearchForFacets | SearchForHits;

What's SearchForHits? It's:

export type SearchForHits = SearchForHitsOptions & SearchParams;

What's SearchForHitsOptions? It just goes on and on and on, all in different files, this endless stack of self-referencing turtles all the way down.

I would have stayed on v4 but our build broke for unknown reasons so I finally spent a few hours yesterday migrating.

Please improve these types so that humans can read and understand them!

shortcuts commented 2 months ago

I just finished migrating our own codebase from Algolia v4 to v5 and found the process very confusing. There's one page that I found with "upgrade" examples and it felt pretty anemic.

Thanks for trying out the new major version still :D We are still working on the docs and improving the clients, sorry it was a complicated migration, and thanks a lot for the feedbacks

The main problem IMHO is that the (new?) TypeScript types are extremely convoluted and hard to follow. For example, when I Command+Click to see what the definition of searchSingleIndex is (because I've never heard of that method before), I get this type definition:

Seems like the issue comes from the spread here, without it the LSP properly finds the definition, that's definitely something we need to fix

It looks auto-generated, no line breaks for clarity, no comments. I've never seen imports used as type parameters - probably because a human would never write this! I see this (buried in a soup of other typedefs/imports) and I want to give up.

You are right, it is generated, but that's not a generation issue, see the definition without the spread:

Screenshot 2024-08-28 at 17 28 46

Or when I try to figure out what options are available in SearchQuery which is the main type for interacting with Algolia. Command+Click, and I get:

export type SearchQuery = SearchForFacets | SearchForHits;

What's SearchForHits? It's:

export type SearchForHits = SearchForHitsOptions & SearchParams;

What's SearchForHitsOptions? It just goes on and on and on, all in different files, this endless stack of self-referencing turtles all the way down.

We reuse types and have polymorphic APIs so it's expected to see unions and intersections. We can try to see if the output is better without reusing types, it would reduce the composability (like having to manually Pick some fields), but if it provides a better DX, why not!

shortcuts commented 1 month ago

The main problem IMHO is that the (new?) TypeScript types are extremely convoluted and hard to follow. For example, when I Command+Click to see what the definition of searchSingleIndex is (because I've never heard of that method before), I get this type definition:


searchSingleIndex<T>({ indexName, searchParams }: import("../model").
SearchSingleIndexProps, requestOptions?: import("@algolia/client-common").RequestOptions):
Promise<import("../model").SearchResponse<T>>;

Hey, the 5.3.0 version (will be published later today) should fix this

MattIPv4 commented 1 month ago

👋 @shortcuts I see that 5.3.0 has been released, but https://www.algolia.com/doc/libraries/javascript/v5/upgrade/ appears to still be incredibly lacklustre with very little in the way of documentation on how to actually migrate usage from v4 (and still references Python methods!). Please could I ask you put some more effort into actually writing a migration guide that covers how folks should update their usages, 'cause methods are definitely not 1:1...? I do not feel anything I mentioned in https://github.com/algolia/algoliasearch-client-javascript/issues/1537#issuecomment-2297759303 has really been addressed.

shortcuts commented 1 month ago

Hey @MattIPv4 thanks for trying out the new major :)

I resolved the issue because the typing problem should be solved, but the but the documentation problem still remains indeed.

Our documentation team is working on writing the new (migration) guides as well as providing a mapping table of the method changes.

In the meantime, maybe the following resources could help:

(sorry again for the documentation issues)

MattIPv4 commented 1 month ago

Thanks, glad to hear some better migration docs are on there way! Is there an issue tracking that that I could follow for updates, or will updates be shared here?