feat(javascript): add cache layer #274

shortcuts · 2022-03-22T14:29:12Z

🧭 What and Why

🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-176

Changes included:

Copy the cache logic from the current client to our client-common folder
- Only type changes have been done
Copy existing tests to our client-common/src/__tests__ folder
- Mostly some method and type changes
- As it's not part of the CTS, I thought it was a good addition. I'll make those run along with the CTS in a later PR.
Update templates to reflect the cache usage
Bump package size to their closest size limit

Next step:

Run the `client-common` tests along with the `CTS`. #279

Diagram for fun

flowchart LR
A[Previous jobs..] --> B[Clients jobs] --> C{Tests}
C -->D[CTS]
C -->E[client-common tests]
D --> F[Codegen]
E --> F[Codegen]

Implement `requests` and `responses` cache in the `transporter` #281

🧪 Test

CI :D
Generated branch

netlify · 2022-03-22T14:29:18Z

✅ Deploy Preview for api-clients-automation canceled.

Name	Link
🔨 Latest commit	`e8a1a69`
🔍 Latest deploy log	https://app.netlify.com/sites/api-clients-automation/deploys/623b2e3392b596000861ff13

shortcuts · 2022-03-22T14:31:43Z

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the generated/main branch.

eunjae-lee · 2022-03-22T16:50:00Z

templates/javascript/api.mustache

@@ -43,6 +44,9 @@ export function {{apiName}}Api(
    },
    requester: options?.requester ?? createHttpRequester(),
    userAgents: [{ segment: 'Node.js', version: process.versions.node }],
+    responsesCache: createNullCache(),
+    requestsCache: createNullCache(),
+    hostsCache: createMemoryCache(),


Can you remind me what hostsCache is?

Too many types of cache, and I'm quite puzzled.

It contains the hosts that responded with timeout or down status: https://github.com/algolia/api-clients-automation/blob/main/clients/algoliasearch-client-javascript/packages/client-common/src/createTransporter.ts#L177-L185

I will store whether a algolia host is down or not, to stop calling it if it's down, and it's used to randomize which host is used also

eunjae-lee · 2022-03-22T16:50:56Z

clients/algoliasearch-client-javascript/packages/algoliasearch/builds/browser.ts

@@ -39,6 +47,14 @@ export function algoliasearch(
    requester: options?.requester ?? createXhrRequester(),
    userAgents: [{ segment: 'Browser' }],
    authMode: 'WithinQueryParameters',
+    responsesCache: createMemoryCache(),


I'm not sure if memory cache should be the default value here. Probably null cache for all (response, request, host for both node and browser). What do you think?

What do we already have for the v4 js client?

It's the same here as the v4 js client, which seems to work well but I don't know much about the limitations here

eunjae-lee · 2022-03-22T16:52:45Z

...algoliasearch-client-javascript/packages/client-common/src/createBrowserLocalStorageCache.ts

+
+        namespace[JSON.stringify(key)] = value;
+
+        getStorage().setItem(namespaceKey, JSON.stringify(namespace));


I'm worried about what happens if the local storage is full. Of course, it's not about this pull-request. I'm just throwing a topic that we should think about in a separate pull-request.

millotp

I skimmed through the actual cache code because it's well tested.
However you are not using the cache in the transporter, is it on purpose ?

clients/algoliasearch-client-javascript/packages/client-common/jest.config.ts

millotp · 2022-03-22T16:40:54Z

clients/algoliasearch-client-javascript/packages/client-common/src/__tests__/null-cache.test.ts

@@ -0,0 +1,49 @@
+import { createNullCache } from '../createNullCache';


For me it makes more sense to have src and tests folders at the root, instead of src/__tests__/, but we need to define it across the entire repo

Yep make sense, we could change all of them together once defined

I personally prefer to have test files as close as their source files. It's too troublesome to import like

import { something } from '../../../../tests/some/nested/folder'

Could even be https://twitter.com/antfu7/status/1505299814069977088 👀

@shortcuts haha yeah I saw that, and thought interesting!

millotp · 2022-03-22T16:43:58Z

templates/javascript/api-all.mustache

@@ -45,6 +46,14 @@ export function {{apiName}}Api(
    requester: options?.requester ?? createXhrRequester(),
    userAgents: [{ segment: 'Browser' }],
    authMode: 'WithinQueryParameters',
+    responsesCache: createMemoryCache(),


The cache should be configurable through the options param IMO, for both node and browser

Indeed, and I think I've recently saw some people using their own it so it definitely make sense

What about this change for the review ?

I'm currently exposing the requestOptions to each methods of the clients so I'll introduce it with it in a later PR!

shortcuts · 2022-03-22T16:56:48Z

I skimmed through the actual cache code because it's well tested. However you are not using the cache in the transporter, is it on purpose ?

It introduced a lot of changes so I'm writing the stacked PR atm, I shouldn't have put the PR ready for review yet 😓

millotp · 2022-03-22T16:58:53Z

Ah ok don't worry !

millotp · 2022-03-23T13:00:39Z

clients/algoliasearch-client-javascript/packages/client-common/jest.config.ts

  preset: 'ts-jest',
-  testEnvironment: 'jsdom',
+  roots: ['src/__tests__/cache'],


roots would make more sense with src/__tests__ right ?

Since we only have cache tests for now I've just left it like that, but indeed

fix here boyz df85f88

millotp

💯

algolia/api-clients-automation#274 Co-authored-by: Clément Vannicatte <20689156+shortcuts@users.noreply.github.com>

feat(javascript): add cache layer

be0aef9

shortcuts marked this pull request as draft March 22, 2022 14:29

shortcuts added 2 commits March 22, 2022 15:53

Merge branch 'main' into feat/APIC-176/javascript-cache

4400bd6

should fix utils cache

adb9896

shortcuts self-assigned this Mar 22, 2022

shortcuts requested review from a team, eunjae-lee and damcou and removed request for a team March 22, 2022 15:28

shortcuts marked this pull request as ready for review March 22, 2022 15:28

shortcuts mentioned this pull request Mar 22, 2022

chore: make CI run JavaScript client-common test #279

Merged

eunjae-lee reviewed Mar 22, 2022

View reviewed changes

millotp reviewed Mar 22, 2022

View reviewed changes

use subfolder to make package clearer, fix jest testEnvironment

7aeba74

shortcuts requested review from millotp and eunjae-lee March 23, 2022 11:45

Merge branch 'main' into feat/APIC-176/javascript-cache

097c3fd

millotp reviewed Mar 23, 2022

View reviewed changes

fix roots path

df85f88

shortcuts requested a review from millotp March 23, 2022 14:25

millotp previously approved these changes Mar 23, 2022

View reviewed changes

shortcuts enabled auto-merge (squash) March 23, 2022 14:26

chore: make CI run JavaScript client-common test (#279)

e8a1a69

shortcuts dismissed millotp’s stale review via e8a1a69 March 23, 2022 14:26

millotp approved these changes Mar 23, 2022

View reviewed changes

shortcuts mentioned this pull request Mar 23, 2022

feat(javascript): use responses and requests cache #281

Merged

shortcuts mentioned this pull request Mar 23, 2022

feat(javascript): expose requestOptions and cache options #283

Merged

shortcuts merged commit 2e7f208 into main Mar 23, 2022

shortcuts deleted the feat/APIC-176/javascript-cache branch March 23, 2022 19:19

eunjae-lee pushed a commit to algolia/algoliasearch-client-javascript that referenced this pull request Mar 23, 2022

feat(javascript): add cache layer

9f638e1

algolia/api-clients-automation#274 Co-authored-by: Clément Vannicatte <20689156+shortcuts@users.noreply.github.com>

This was referenced Apr 1, 2022

chore: release 2022-04-01 #320

Closed

chore: release 2022-04-01 #322

Closed

chore: release 2022-04-01 #324

Closed

chore: release 2022-04-05 #333

Closed

chore: release 2022-04-05 #334

Closed

chore: release 2022-04-05 #335

Closed

shortcuts added a commit that referenced this pull request Apr 22, 2022

feat(javascript): add cache layer (#274)

b709249

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(javascript): add cache layer #274

feat(javascript): add cache layer #274

shortcuts commented Mar 22, 2022 •

edited

Loading

netlify bot commented Mar 22, 2022 •

edited

Loading

shortcuts commented Mar 22, 2022 •

edited

Loading

eunjae-lee Mar 22, 2022

shortcuts Mar 22, 2022

millotp Mar 22, 2022

eunjae-lee Mar 22, 2022

shortcuts Mar 22, 2022

eunjae-lee Mar 22, 2022

millotp left a comment

millotp Mar 22, 2022

shortcuts Mar 22, 2022

eunjae-lee Mar 23, 2022

shortcuts Mar 23, 2022

eunjae-lee Mar 24, 2022

millotp Mar 22, 2022

shortcuts Mar 22, 2022

millotp Mar 23, 2022

shortcuts Mar 23, 2022

shortcuts commented Mar 22, 2022

millotp commented Mar 22, 2022

millotp Mar 23, 2022

shortcuts Mar 23, 2022

shortcuts Mar 23, 2022

millotp left a comment


		namespace[JSON.stringify(key)] = value;

		getStorage().setItem(namespaceKey, JSON.stringify(namespace));

		@@ -0,0 +1,49 @@
		import { createNullCache } from '../createNullCache';

feat(javascript): add cache layer #274

feat(javascript): add cache layer #274

Conversation

shortcuts commented Mar 22, 2022 • edited Loading

🧭 What and Why

Changes included:

Next step:

Run the client-common tests along with the CTS. #279

Implement requests and responses cache in the transporter #281

🧪 Test

netlify bot commented Mar 22, 2022 • edited Loading

✅ Deploy Preview for api-clients-automation canceled.

shortcuts commented Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millotp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shortcuts commented Mar 22, 2022

millotp commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millotp left a comment

Choose a reason for hiding this comment

shortcuts commented Mar 22, 2022 •

edited

Loading

Run the `client-common` tests along with the `CTS`. #279

Implement `requests` and `responses` cache in the `transporter` #281

netlify bot commented Mar 22, 2022 •

edited

Loading

shortcuts commented Mar 22, 2022 •

edited

Loading