Capture just one callback per result

[bvicenzo]

Nowadays, if we are catching the result we have to take care about the callback matching with more than on callback for one result...

Ex:

Service.(args).on_failure do |error, type|
  flash[:error] = if type == :invalid_attributes
                    error.full_messages.to_sentence
                  else
                    'Não foi possível importar os dados, tente novamente.'
                  end
end

With this PR implementation it could be done as bellow:

Service.(args)
   .on_failure(:invalid_attributes) { |error, _type| flash[:error] = error.full_messages.to_sentence }
   .on_failure { 'Não foi possível importar os benefícios, tente novamente.' }

Then, basically, on this suggestion, the callback propagation stops when some callback matches with the error or success.

[MatheusRich]

I love this addition. I'd argue that we shouldn't solve this problem by not propagating results, though. I think it's useful to have a way to run a callback on any failure:

Service.(args)
   .on_failure(:error_1) { do_stuff }
   .on_failure(:error_2) { do_other_stuff }
   .on_failure { "This runs on any failure" } # error_1 and error_2 are failures, it's confusing not running this

Also, changing this is a breaking change! Maybe we can have additional callbacks?

Service.(args)
   .on_failure(:error_1) { do_stuff }
   .on_failure(:error_2) { do_other_stuff }
   .on_unhandled_failure { "This only runs on unhandled failures" }
   .on_failure { "This runs on any failure" }

or maybe

Service.(args)
   .on_failure(:error_1) { do_stuff }
   .on_failure(:error_2) { do_other_stuff }
   .on_failure(unhandled: true) { "This runs on unhandled failures" }
   .on_failure(:unhandled) { "This runs on unhandled failures" } # we could use a symbol to make it shorter, but it would be a small breaking change as well

[bvicenzo]

Man, I like to mutch about this point...

My main concern about this point are:

• It seems that people are aware of not passing args it matches anything;
• Using a word to specify a * matcher, it looks like we would have to work with a reserved keyword, haven't we?

For example:

class MyService < FService
  def run
    Failure(:unhandled) # It would raise something like a Reserved keyword because it'll be used as `*` in `on_failure`, right?
  end
end

Makes it sense?
It's ok, to me to work with a reserved word... and you, guys?

[MatheusRich]

@bvicenzo Yeah using a symbol would make unhandled a reserved type. That's why I suggested using a keyword argument .on_failure(unhandled: true) { "This runs on unhandled failures" }. It's also important to replicate the behavior on on_success as well

[bvicenzo]

@MatheusRich Thinking a little bit more, here?
Wdyta if instead it to be an argument, we have other methods to handle unhandled success or failure?
Ex:

Service.(args)
  .on_success(:ok) { 'Ok operation' }
  .on_unhundled_success { 'a generic message' }
  .on_failure(:error, :other_error) { 'Error occurred' }
  .on_unhundled_failure { 'An unknown error happened here' } 

Then, we don't need to reserve any word... 🤔

[MatheusRich]

@bvicenzo Yeah, I mentioned that on my first comment. It's a bit more verbose, but I think it's an easy addition and doesn't break compatibility. It's an interesting opportunity to experiment with it and come up with better naming in the future.

[matheusbsilva]

I agree with @MatheusRich suggestion, specially because it doesn't break compatibility. I'm not happy with the names, but I didn't think in something better, so it's fine.

[bvicenzo]

@MatheusRich @matheusbsilva @euricovidal Could you take a look again?
I´ve tried to implement the option:

Service.(args)
   .on_failure(:error_1) { do_stuff }
   .on_failure(:error_2) { do_other_stuff }
   .on_failure(unhandled: true) { "This runs on unhandled failures" }

e305816

[MatheusRich]

Can you clarify why are we warning on this? I think having a plain on_success { do_x }/on_failure { do_x } is okay, since not always we will use result types.

[bvicenzo]

Well, the problem we were wanting to solve here was exactly that...

• The callback propagation never stops even if some callback processes the result;
• The callback without args matches everything;

Then if we run this...

User::Update.(user: user)
  .on_success { |value| return json_success(value) }
  .on_failure(:error_1) { |value| return catch_error_1(value) }
  .on_failure(:error_2) { |value| return catch_error_2(value) }
  .on_failure { |value| return catch_any_unknown_error(value) }

Even if error 1 or 2 is processed, the unknown error will be processed as well.

Then, the PR proposed just to add a stop propagation to if error 1 or 2 process the result the * matcher does not process anything...

Then, you suggested (if I understood well), to change the on_(failure/success) to add an argument to indicate that it matches anything unhandled until that callback runs.

Then, seems that does not make sense to have 2 ways to process any unhandled result, because...

User::Update.(user: user)
  .on_success { |value| return json_success(value) }
  .on_failure(:error_1) { |value| return catch_error_1(value) }
  .on_failure(:error_2) { |value| return catch_error_2(value) }
  .on_failure { |value| return catch_any_unknown_error(value) } # Process any unhandled result
  .on_failure(unhandled: true) { |value| return catch_any_unknown_error(value) } # Will never run

User::Update.(user: user)
  .on_success { |value| return json_success(value) }
  .on_failure(:error_1) { |value| return catch_error_1(value) }
  .on_failure(:error_2) { |value| return catch_error_2(value) }
  .on_failure(unhandled: true) { |value| return catch_any_unknown_error(value) } # Process any unhandled result
  .on_failure { |value| return catch_any_unknown_error(value) } # Will never run also

This, to me, could result in more confusion than let as it was and use just as it was (passing no arguments)...

That's why without any args becomes deprecated.

[MatheusRich]

I still think it's valuable to have a plain on_success/on_failure without a type. Many interactors only have two flows: failure and success, so requiring a type is cumbersome and error-prone (imagine misspelling an error type .on_failure(:user_not_fonud))

One alternative would be making this configurable Gem-level: i.e., allowing users to define if on_success/on_failure should run only on unhandled failures or not.

FService.config do |config|
  config.callbacks_run_only_on_unhandled_results = true
end

I think this is a bit too much to my taste, but I imagine y'all ran into some kind of bug due to this.

[j133y]

@bvicenzo