fix: fetch all references with includes

[ChidinmaOrajiaku]

Purpose

Fetch all references with includes

Screen.Recording.2024-03-25.at.09.31.20.mov

With Deep Binding

Screen.Recording.2024-03-25.at.09.33.29.mov

Approach

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
experience-builder-test-app	✅ Ready (Inspect)	Visit Preview	Mar 26, 2024 1:53pm

[dimitrycf]

Will properly test it after PH, but a quick open question: does it "deduplicate" the includes{} ?

because, I wonder if each page will bring it's own includes{} with let's say asset123 and then in theory if 10 pages bring in asset123 as part of includes{} it will be in final includes{} 10 times?

[ChidinmaOrajiaku]

Will properly test it after PH, but a quick open question: does it "deduplicate" the includes{} ?

because, I wonder if each page will bring it's own includes{} with let's say asset123 and then in theory if 10 pages bring in asset123 as part of includes{} it will be in final includes{} 10 times?

This is up to the API, but in theory, I guess your thought is correct.

[dimitrycf]

Hey Didi! 👋

As I was trying to understand the algorithm I was making some notes, and noticed that the algorithm can be simplified and certain operations can be made explicit.

Please have a look at this proposal for the refactoring and let me know what you think.

PR #522

[dimitrycf]

I have noticed that you intorduced separation between fetchAllEntries() and fetchAllAssets(). I tried that too on Friday (I tried refactoring, but got stuck with wrangling TypeScript types for an hour or so and eventually realized that rolling back was the easier way for a Friday evening solution).

However, now that there are two functions, we have this pretty complicated logic of recursive halving duplicated 😱 that makes me scared!

  } catch (error) {
    if (
      error instanceof Error &&
      error.message.includes('size too big') &&
      limit > MIN_FETCH_LIMIT
    ) {
      const newLimit = Math.max(MIN_FETCH_LIMIT, Math.floor(limit / 2));
      return fetchAllAssets({
        client,
        ids,
        locale,
        skip,
        limit: newLimit,
        responseItems,
      });
    }

    throw error;
  }

[dimitrycf]

It would be great to add a comment as to why we compare with 6, eg.

// Five pages will return those values of `includes.Entry[]` 
// Page 1 : `some-entry-id` , `some-entry-id-0`
// Page 2: `some-entry-id` , `some-entry-id-20`
// Page 3: `some-entry-id` , `some-entry-id-40`
// Page 4: `some-entry-id` , `some-entry-id-60`
// Page 5: `some-entry-id` , `some-entry-id-80`
// But the `result.includes.Entry` is expected to only return unique entries for consistency
// And there are only 6 uniques, as duplicates of `some-entry-id` are not returned or counted
expect(result.includes.Entry).toHaveLength(6);

[dimitrycf]

Video validation
https://www.loom.com/share/4371eb19d2bc4a97bb34e0f7c551cabf

[dimitrycf]

Also would be great to add validation of total count to calls to :

• fetchAllEntries()
• fetchAllAssets()

something like this:

const result = await fetchAllEntries(params);
expect(result.items).toHaveLength(100);

[dimitrycf]

Maybe we can add also test for total amount of items eg.

const result = await fetchAllEntries(params);
expect(result.items).toHaveLength(100);

[dimitrycf]

APPROVING TO UNBLOCK

(see the comments with suggestions)