Skip to content
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

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

Merged
merged 9 commits into from
Mar 24, 2022

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented Mar 23, 2022

🧭 What and Why

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

Changes included:

Follow up of #274

  • Use responses and requests cache in createTransporter
  • Update bundlesize
  • Move everything under a transporter folder

Next step

Import transporter tests from the existing suite

🧪 Test

CI :D

@netlify
Copy link

netlify bot commented Mar 23, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 7d6c155
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/623c34fcd70358000907f6d8

@shortcuts
Copy link
Member Author

shortcuts commented Mar 23, 2022

✗ The generated branch has been deleted.

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

@shortcuts shortcuts force-pushed the feat/APIC-176/use-javascript-cache branch from 8d47222 to 46cbe6a Compare March 23, 2022 15:31
@shortcuts shortcuts self-assigned this Mar 23, 2022
Base automatically changed from feat/APIC-176/javascript-cache to main March 23, 2022 19:19
@shortcuts shortcuts marked this pull request as ready for review March 24, 2022 08:43
@shortcuts shortcuts requested review from a team, eunjae-lee and damcou and removed request for a team March 24, 2022 08:43
@shortcuts shortcuts enabled auto-merge (squash) March 24, 2022 08:55
@shortcuts shortcuts disabled auto-merge March 24, 2022 09:08
request: Request,
requestOptions: RequestOptions
): Promise<TResponse> {
if (request.method !== 'GET') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it the same condition as what we already have in v4?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, except that the v4 have read and write request, here we have a single one and early exit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/algolia/algoliasearch-client-javascript/blob/master/packages/transporter/src/createTransporter.ts#L26

I cannot find anything about non- GET methods. We use lots of POST methods, and we won't be able to use cache for POST method?

Copy link
Collaborator

@millotp millotp Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Responses to POST Request are not cacheable according to the HTTP spec.
Maybe the request is

Copy link
Member Author

@shortcuts shortcuts Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, basically you should only cache GET requests.

This is reflected in v4 by splitting the request method in two: read and write

You can search for MethodEnum.Get in the codebase, or Post, etc.

Copy link
Contributor

@eunjae-lee eunjae-lee Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's what it meant by read and write there. yeah, let's go with this for the compatibility.

However, now that we're already using POST very heavily to get data, not allowing cache for POST was probably a mistake in v4.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it was more a matter of following good practices, this is (or was?) recommended to invalidate cache for write requests

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if on the engine side there's a reason to use post for search

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, with GET method, there's a small limitation on url query parameters, where we tend to send a huge query string (think about InstantSearch), so we sort of had to use POST, which is not intended for the purpose, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah that make sense

@shortcuts shortcuts enabled auto-merge (squash) March 24, 2022 09:27
@shortcuts shortcuts requested a review from eunjae-lee March 24, 2022 09:27
@shortcuts shortcuts merged commit 272ebd3 into main Mar 24, 2022
@shortcuts shortcuts deleted the feat/APIC-176/use-javascript-cache branch March 24, 2022 09:43
@eunjae-lee eunjae-lee mentioned this pull request Mar 24, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants