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(clients): support recommend methods in algoliasearch #2834

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aymeric-giraudet
Copy link
Member

@aymeric-giraudet aymeric-giraudet commented Mar 7, 2024

🧭 What and Why

As we're trying to unify FX libraries, we want to avoid requiring users to import a bunch of different packages.
We've made those changes to v4 : algolia/algoliasearch-client-javascript#1509

🎟 JIRA Ticket: FX-2774

Changes included:

I'm publishing this as a draft as I'm overall not too sure about the best way to implement this :

  • I opted to merge the search and recommend specs for algoliasearch/lite, and it works well however this will have repercussions for the Dart client (cc @aallam)
  • For the algoliasearch part (not lite), I'm a bit torn as methods are not exported so we can't have the same treeshaking we have with v4, createRecommend has to be called and the bundle will have redundant code. Running into problems with similar models being exported too.
    Also didn't update it for Dart but it might be fine for now.

🧪 Test

Didn't bother with tests yet as this is subject to change :D

@algolia-bot
Copy link
Collaborator

algolia-bot commented Mar 7, 2024

🔨 The codegen job will run at the end of the CI.

Make sure your last commit does not contain generated code, it will be automatically pushed by our CI.

@aymeric-giraudet aymeric-giraudet changed the title feat(algoliasearch): support recommend methods feat(clients): support recommend methods in algoliasearch Mar 7, 2024
const parsed = yaml.load(await fsp.readFile(toAbsolutePath(bundledPath), 'utf8')) as Spec;
const base = yaml.load(await fsp.readFile(toAbsolutePath(bundledPath), 'utf8')) as Spec;

await run(`yarn specs:fix recommend`);
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'm aware this could be handled better by parallelizing with Promise.all and whatnot, but I feel it's not worth complexifying the code for

Comment on lines +202 to +205
const recommendTmpPath = toAbsolutePath('specs/bundled/algoliasearch-recommend.tmp.yml');
await run(`yarn openapi bundle specs/recommend/spec.yml -o ${recommendTmpPath} --ext yml`);
const recommend = yaml.load(await fsp.readFile(recommendTmpPath, 'utf8')) as Spec;
fsp.rm(recommendTmpPath);
Copy link
Member Author

Choose a reason for hiding this comment

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

There might be a way to use the openapi package to load/bundle a YML spec without having to write a temp file or use shell

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

thanks for the contribution! I actually had a local branch for this feature, I'll add them to your branch and push so we can discuss it :)

@@ -26,6 +26,7 @@
{{> algoliasearch/builds/initClients}}

return {
...createRecommendClient(commonOptions),
Copy link
Member

Choose a reason for hiding this comment

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

we have to use the init solution like for analytics etc. otherwise both base clients will be merged, I think we can leave search as the base client it would make things clearer for users

@shortcuts
Copy link
Member

thanks for the contribution! I actually had a local branch for this feature, I'll add them to your branch and push so we can discuss it :)

had trouble merging our changes, so I've applied some of yours on my PR, which should make the test pass #2860, let me know wdyt

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.

None yet

3 participants