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

Define callbacks mode as a lambda #1583

Open
stengineering0 opened this issue Sep 19, 2022 · 0 comments
Open

Define callbacks mode as a lambda #1583

stengineering0 opened this issue Sep 19, 2022 · 0 comments

Comments

@stengineering0
Copy link

stengineering0 commented Sep 19, 2022

Is your feature request related to a problem? Please describe.
Currently the suggested way to perform the reindexing conditionally is

class Product < ApplicationRecord
  searchkick callbacks: false

  # add the callbacks manually
  after_commit :reindex, if: -> (model) { model.previous_changes.key?("name") } # use your own condition
end

The problem is that this approach is likely about the copy/paste action from Searchkick internals into the host application. The client is forced to investigate the sources of Searchkick, understand in details how and why it uses after_commit callback, paste the same into own application, apply the customizations, and as it always happen with code duplication - keep tracking the upstream sources in order to catch & reflect the breaking changes.

For instance, the suggested conditional reindexing approach from above relies on :inline callbacks mode under the hood. If a client normally uses :async and just wants to skip reindexing sometimes - it have to do something like:

class Product < ApplicationRecord
  searchkick callbacks: false

  # add the callbacks manually
  after_commit(if: -> (model) { model.previous_changes.key?("name") } { reindex(mode: :async) }
end

And still some important parts or Searchkick functionality might be broken now (or will be broken later after Searchkick upgrade) - for instance the global callbacks mode. This way the full-weight suggestion turns into:

class Product < ApplicationRecord
  searchkick callbacks: false

  # add the callbacks manually
  after_commit(if: -> (model) { Searchkick.callbacks? && model.previous_changes.key?("name") } { reindex(mode: :async) }
end

And still it does not fully respect the global callbacks mode such as Searchkick.callbacks(:bulk) do; end..

Describe the solution you'd like
Let's introduce the possibility to define callbacks mode as lambda for the model as follows:

class Product < ApplicationRecord
  searchkick callbacks: ->{ searchkick_callbacks_mode }

  def searchkick_callbacks_mode
    return false unless previous_changes.key?("name")
    return :inline if urgent?

    :async
  end
end

It will be evaluated within generic after_commit Searchkick callback.

I'm ready to provide PR once the feature will be accepted in general by code owners )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant