sanity-io / sanity-algolia

Utilities for indexing Sanity documents in Algolia
MIT License
67 stars 16 forks source link

Map a Sanity document to multiple Algolia records #14

Closed fabien closed 1 year ago

fabien commented 3 years ago

This PR covers the following:

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

fabien commented 3 years ago

Oops, the array handling is already part of this PR https://github.com/sanity-io/sanity-algolia/pull/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 commented 3 years ago

@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 commented 3 years ago

This last commit adds bit of a safety net to run the code shown here https://github.com/sanity-io/sanity-algolia/issues/12#issuecomment-842622162 without a hitch.

fabien commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

fabien commented 3 years ago

@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 commented 3 years ago

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.