feat(javascript): add browser xhr requester #115

shortcuts · 2022-02-03T12:24:51Z

🧭 What and Why

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

Changes included:

This PR introduces an XHR requester for browser builds and remove usage of classes in our clients.

Move HttpRequester to its own requester-node-http package.
Introduce XhrRequester in requester-browser-xhr package.
Inferred return type of our client, which should be handled by a generated type later if needed. (See Next steps)
Generated clients now output node.ts and browser.ts that will be used by our bundler to generate umd and esm files.
Remove usage of classes, we now return a client with generated methods.
Update CTS/Playground accordingly

Limitations:

Generator

As of now, we use a tweaked predefined openapi generator template, which does not allow us to provide external files or change file name.

I've then added a post-gen script for JavaScript to rename unused template files so we can use them to generate our node.ts and browser.ts build files.

api.mustache output is renamed to node.ts after a successful generation
api-all.mustache output is renamed to browser.ts after a successful generator

ESLint

Some of our parameters have the same name as the function, weirdly, the no-shadow option does not work in this case, so we only warn this error in the meantime.

Next steps:

Generated return type of our clients (with method typing)
Remove classes usage for XhrRequester and HttpRequester (this PR is already too big)
Bundling for umd and esm
(?) Create our own template to avoid the rename thing

🧪 Test

CI :D

tests/output/javascript/tests/client/analytics.test.ts

eunjae-lee · 2022-02-03T14:32:37Z

tests/output/javascript/tests/client/search.test.ts

@@ -1,26 +1,21 @@
 // @ts-nocheck
-import { SearchApi, EchoRequester } from '@algolia/client-search';
+import { EchoRequester } from '@algolia/client-common';
+import { searchApi } from '@algolia/client-search';


what if we drop Api and just call it search? Seeing Api on the end users' source code might be weird.

I think we've kept the Api because the generator forces it somewhere in the template, and it also makes the naming match across all API clients.

I can try to change that if other agree on this naming cc @millotp @damcou

Anyway, I was just triggering a discussion, and it doesn't have to happen in this PR, so it's not a blocker. But I'm still wondering about what others think about the renaming.

Definitely a good point!

I would be fine with just search but at the same time it would collide with a lot of others interface/var that it might be counter productive

eunjae-lee

looks good to me!

bodinsamuel

LGTM
Minor comments

NB: be mindful of those reviewing, this PR could have been split

bodinsamuel · 2022-02-04T11:02:50Z

clients/algoliasearch-client-javascript/client-search/src/searchApi.ts

-  }
+    ] as Host[]
+  ).concat(
+    shuffle([


I would not do the shuffle here, this makes the get unpredictable for testing (I guess it's done like to highlight that we should not pick host directly but that's an advanced use case anyway)

bodinsamuel · 2022-02-04T11:06:58Z

clients/algoliasearch-client-javascript/recommend/src/recommendApi.ts

+    baseHeaders: {
+      'content-type': 'application/x-www-form-urlencoded',
+    },
+    userAgent: options.userAgent,


We should have a default value for userAgent otherwise people using the create<N>Api will never setup the appropriate userAgent.
And we could have a getDefaultUserAgent to make it portable/testable

otherwise people using the createApi will never setup the appropriate userAgent

True, but I'm also not sure if it make sense to export the create function actually 🤔 wdyt

we could have a getDefaultUserAgent to make it portable/testable

Definitely

Makes more sense like this indeed: db68d10. Is that what you had in mind?

clients/algoliasearch-client-javascript/requester-browser-xhr/package.json

templates/javascript/package.mustache

bodinsamuel · 2022-02-04T11:13:05Z

tests/output/javascript/tests/client/search.test.ts

@@ -1,26 +1,21 @@
 // @ts-nocheck
-import { SearchApi, EchoRequester } from '@algolia/client-search';
+import { EchoRequester } from '@algolia/client-common';
+import { searchApi } from '@algolia/client-search';


I would be fine with just search but at the same time it would collide with a lot of others interface/var that it might be counter productive

shortcuts · 2022-02-04T11:18:53Z

@bodinsamuel

NB: be mindful of those reviewing, this PR could have been split

True, but at the same time it would have made reviewers check dead code (e.g. only adding xhr requester -> moving to classes), so I've went will the full package, sorry!

feat(js): add browser xhr requester

bb71960

shortcuts requested a review from bodinsamuel February 3, 2022 12:24

shortcuts self-assigned this Feb 3, 2022

shortcuts removed the request for review from bodinsamuel February 3, 2022 12:24

shortcuts force-pushed the feat/APIC-287/javascript-xhr-requester branch from 88ab698 to 99730b8 Compare February 3, 2022 12:34

GitHub actions

c2969a8

shortcuts force-pushed the feat/APIC-287/javascript-xhr-requester branch from 99730b8 to c2969a8 Compare February 3, 2022 12:36

shortcuts requested review from eunjae-lee, bodinsamuel and millotp and removed request for eunjae-lee February 3, 2022 12:43

shortcuts marked this pull request as ready for review February 3, 2022 12:43

shortcuts added 2 commits February 3, 2022 14:27

Merge branch 'main' into feat/APIC-287/javascript-xhr-requester

38a32ae

Merge branch 'main' into feat/APIC-287/javascript-xhr-requester

6ad642c

eunjae-lee reviewed Feb 3, 2022

View reviewed changes

Generate type from ReturnType

555206c

shortcuts requested a review from eunjae-lee February 3, 2022 15:17

regen tests

49bba60

eunjae-lee previously approved these changes Feb 3, 2022

View reviewed changes

Merge branch 'main' into feat/APIC-287/javascript-xhr-requester

35e79e1

shortcuts changed the title ~~feat(js): add browser xhr requester~~ feat(javascript): add browser xhr requester Feb 3, 2022

bodinsamuel previously approved these changes Feb 4, 2022

View reviewed changes

add default user agent in createApi

db68d10

shortcuts dismissed stale reviews from bodinsamuel and eunjae-lee via db68d10 February 7, 2022 08:27

shortcuts requested review from bodinsamuel and eunjae-lee February 7, 2022 10:18

bodinsamuel approved these changes Feb 7, 2022

View reviewed changes

eunjae-lee mentioned this pull request Mar 7, 2022

chore: release 2022-03-07 #236

Closed

4 tasks

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

feat(javascript): add browser xhr requester (#115)

c448252

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(javascript): add browser xhr requester #115

feat(javascript): add browser xhr requester #115

shortcuts commented Feb 3, 2022 •

edited

Loading

eunjae-lee Feb 3, 2022

shortcuts Feb 3, 2022 •

edited

Loading

eunjae-lee Feb 3, 2022 •

edited

Loading

shortcuts Feb 3, 2022

bodinsamuel Feb 4, 2022

eunjae-lee left a comment

bodinsamuel left a comment

bodinsamuel Feb 4, 2022

bodinsamuel Feb 4, 2022

shortcuts Feb 4, 2022 •

edited

Loading

shortcuts Feb 7, 2022

bodinsamuel Feb 4, 2022

shortcuts commented Feb 4, 2022

feat(javascript): add browser xhr requester #115

feat(javascript): add browser xhr requester #115

Conversation

shortcuts commented Feb 3, 2022 • edited Loading

🧭 What and Why

Changes included:

Limitations:

Generator

ESLint

Next steps:

🧪 Test

eunjae-lee Feb 3, 2022

Choose a reason for hiding this comment

shortcuts Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

eunjae-lee Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

shortcuts Feb 3, 2022

Choose a reason for hiding this comment

bodinsamuel Feb 4, 2022

Choose a reason for hiding this comment

eunjae-lee left a comment

Choose a reason for hiding this comment

bodinsamuel left a comment

Choose a reason for hiding this comment

bodinsamuel Feb 4, 2022

Choose a reason for hiding this comment

bodinsamuel Feb 4, 2022

Choose a reason for hiding this comment

shortcuts Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

shortcuts Feb 7, 2022

Choose a reason for hiding this comment

bodinsamuel Feb 4, 2022

Choose a reason for hiding this comment

shortcuts commented Feb 4, 2022

shortcuts commented Feb 3, 2022 •

edited

Loading

shortcuts Feb 3, 2022 •

edited

Loading

eunjae-lee Feb 3, 2022 •

edited

Loading

shortcuts Feb 4, 2022 •

edited

Loading