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

Map a Sanity document to multiple Algolia records #14

Closed
wants to merge 10 commits into from

Conversation

fabien
Copy link
Contributor

@fabien fabien commented May 20, 2021

This PR covers the following:

  • SerializeFunction can now be asynchronous, which enables the serialize function to perform further async processing (api-calls, lookups, transforms etc.)
  • The serialize function can return an array, which allows one input document to emit multiple output documents (for example, one for each item in an embedded array of objects within Sanity)
  • The can override default values did not actually override a default, it should have used objectID not objectId

Please let me know if you are willing to merge this - otherwise I will be publishing my fork directly onto NPM soon. Thanks!

@fabien
Copy link
Contributor Author

fabien commented May 20, 2021

Oops, the array handling is already part of this PR #13 - so I guess the same caveats apply here, although the use-case I envision is different. Will look into this further and see how deletion would work in this case.

@fabien
Copy link
Contributor Author

fabien commented May 20, 2021

@runeb @theianjones I've added the useTags option to store the source document id in the _tags property of Algolia. This is a special 'meta' property that can be (ab)used for the purpose of keeping track of which documents to delete. Let me know what you think!

@fabien
Copy link
Contributor Author

fabien commented May 20, 2021

This last commit adds bit of a safety net to run the code shown here #12 (comment) without a hitch.

@fabien
Copy link
Contributor Author

fabien commented Jun 30, 2021

@runeb @kmelve unfortunately I haven't received any feedback yet, and out of necessity I will be publishing a forked version before the end of the week.

@kmelve
Copy link
Member

kmelve commented Jun 30, 2021

Sorry about this @fabien – this slipped under my radar (granted, my GH notifications are flooded). Thanks for the PR! I'll bring this to the attention to the right people.

@runeb
Copy link
Member

runeb commented Jun 30, 2021

Hello @fabien and thank you for your contributions! Apologies about not getting back to you sooner.

The features you have added are good and I am positive about merging them. I just need some time to properly go through the PR as it is somewhat lengthy. I have a few reservations on the replaceAll and deleteByQuery options / naming. I think this can be made a little clearer for the users.

To unblock you I have cut a pre-release version of your PR at 1.1.0-alpha
https://github.com/sanity-io/sanity-algolia/releases/tag/1.1.0-alpha
https://www.npmjs.com/package/sanity-algolia/v/1.1.0-alpha

@fabien
Copy link
Contributor Author

fabien commented Jun 30, 2021

Hi @runeb, thanks for your kind reply and the alpha release! I agree, we should see if we can improve the naming of these options. I'm open to any suggestions.

@runeb
Copy link
Member

runeb commented Jun 30, 2021

@fabien Any reason to use _tags instead of just a custom property? We already add rev and type for instance. And will it cause document _id values to appear in Algolia UI components faceted search etc?

@runeb runeb changed the title Features: async SerializeFunction, map array of docs, tiny test fix Features: map array of docs, tiny test fix Jun 30, 2021
@runeb
Copy link
Member

runeb commented Jun 30, 2021

Ideally this should be split into multiple PRs. That would make it easier to review.

  1. Async serialize function ✅
  2. Multiple Algolia records per Sanity document, with _tags or similar strategy for keeping track
  3. Full reindexing
  4. Options, including custom fetch params

I've cherry picked and merged the async serialize commit into main, along with some other small commits from this PR

@runeb runeb changed the title Features: map array of docs, tiny test fix Map a Sanity document to multiple Algolia records Jun 30, 2021
@fabien
Copy link
Contributor Author

fabien commented Jul 1, 2021

@fabien Any reason to use _tags instead of just a custom property? We already add rev and type for instance. And will it cause document _id values to appear in Algolia UI components faceted search etc?

I chose _tags because it has some interesting properties out of the box: https://www.algolia.com/doc/guides/managing-results/refine-results/filtering/how-to/filter-by-tags/#the-difference-between-tags-and-a-custom-array

The _tags reserved attribute is dedicated to filtering. For that reason, you don’t have to set it as an attribute for faceting and use the filterOnly modifier like you would with a custom attribute.

Unless you explicitly include an Algolia InstantSearch widget to filter on _tags it will remain invisible to the end-user.

@fabien
Copy link
Contributor Author

fabien commented Jul 1, 2021

Ideally this should be split into multiple PRs. That would make it easier to review.

  1. Async serialize function ✅
  2. Multiple Algolia records per Sanity document, with _tags or similar strategy for keeping track
  3. Full reindexing
  4. Options, including custom fetch params

I've cherry picked and merged the async serialize commit into main, along with some other small commits from this PR

Agreed, multiple PRs would have been ideal. However, except for the fixes and passing along params (all minor things, even the full indexing), the other features are more or less intertwined. At least that's how they had grown from a real-world need. Eve when it comes to actual LOC changed, it's all pretty minor as a PR - the added test cases far out-weight the code changes, and the overhead a the time to maintain 3-4 feature branches and individual PRs didn't seem worth it.

Please let me know if you need further assistance.

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

4 participants