Returns

[leastbad]

Type of PR (feature, enhancement, bug fix, etc.)

Feature

Description

This PR proposes to introduce RPC-style return values to all Reflex types.

The Reflex class now has an accessor called payload that is initialized to an empty Hash. This Hash is added to the detail object of the morph/inner_html operation, or the event dispatched by broadcast_message. It works a bit like the flash or session objects, in that it accepts a key:

payload[:sign] = 666

If you want to return a single value, you can do so using this syntax:

self.payload = 666

The payload object is passed as a parameter to the client-side promise resolver, making it possible for someone to capture one or several return values from their Reflex in addition to whatever morphing or other functionality.

Why should this be added

I'm currently working on adding Reflex support to my SlimSelect Stimulus controller. It requires that you provide a callback to support "Ajax" lookups:

ajax: (search, callback) => {
    this.stimulate('Example#lookup', search).then(payload => {
      callback(false)
    })
  }
// ...
document.addEventListener('data', event => this.select.setData(event.detail.options))

Then in my contrived Reflex, I broadcast an event that contains the required data, which is picked up by the client.

  def lookup(search)
    result = [{text: "chris", value: 1}, {text: "chip", value: 2}]
    cable_ready.dispatch_event(name: "data", detail: {options: result}).broadcast
    morph :nothing
  end

It's fine... it works. There's nothing wrong with the above... but it does bother me that it requires an extra step on the server and client. Cognitively, the program flow is split between the initial call and the event handler. I tell the Ajax handler that there are no results, and then update the options a few moments later, when the event arrives.

Instead, allowing a Reflex to return a result makes promise resolution much more powerful on the client, and without having to send an extra event:

ajax: (search, callback) => {
    this.stimulate('Example#lookup', search).then(({ payload }) => callback(payload.users))
  }

No extra event dispatch makes for compact Reflex action methods:

  def lookup(search)
    payload[:users] = [{text: "chris", value: 1}, {text: "chip", value: 2}]
    morph :nothing
  end

I think it's pretty cool.

Note that if you wanted to redo this using a single-value payload, it would look like:

ajax: (search, callback) => {
    this.stimulate('Example#lookup', search).then(({ payload }) => callback(payload))
  }

No extra event dispatch makes for compact Reflex action methods:

  def lookup(search)
    self.payload = [{text: "chris", value: 1}, {text: "chip", value: 2}]
    morph :nothing
  end

Checklist

• My code follows the style guidelines of this project
• Checks (StandardRB & Prettier-Standard) are passing
• This is not a documentation update

[marcoroth]

I love it! One concern though: Isn't the name nothing confusing now? Now it's possible that something can be returned, so it's not nothing anymore 😅

[julianrubisch]

I don't see a problem with naming TBH, because still nothing is morphed

[marcoroth]

Okay if you look at it from the morph perspective. But shouldn't it then be possible to add a returns value to the other morph modes as well. That's at least why I would expect. I don't know if that really makes sense, but it wouldn't hurt, right?

Maybe it would anyway make sense that wrap it in a named param:

def lookup(search)
  morph :nothing, returns: [{text: "chris", value: 1}, {text: "chip", value: 2}]
end

In the :selector morph scenario it could also make sense to return something, because you maybe want to do something with the result in the Stimulus controller you are stimulating the reflex:

def card
  morph dom_id(user), render(partial: "users/card", user: user), returns: user.id
end

[leastbad]

I'm long-since on the record for disliking the way we use "nothing". I understand why we do this and I was not able to come up with a better alternative. I wouldn't generally want to change it, but if we're moving to v4 and a great alternative name comes up, I'd likely support the transition, even if we'd likely have to have a deprecation period for the "nothing" term.

I spent time thinking about the other broadcasters and whether they should returns. The thoughts I had were:

1. You're not wrong - it would be handy.
2. I feel weird giving page + select a 3rd parameter, and nothing a 2nd parameter
3. What about changing it to a keyword: parameter?
4. The main reason I didn't do it for the other two is that it would be a huge amount of work, and the end result would require substantial testing... making this PR no longer casual.

The reason is that nothing morphs are similar to errors, in that we piggyback on the mechanism we use to handle/report on errors and halted/aborted Reflexes. We do this because page and selector morphs have their own CableReady operation and broadcast loops. There is no server message event for a morph; we pull the Reflex envelope data out of the morph operations themselves.

In order to make this work for all three morph types, we'd have to look at adding a server-message to the other morph types. It's not necessarily a bad idea. I just don't have a lot of passion for it right now, especially when I'm waiting on so many other things to land. #444 in particular is a major blocker.

[hopsoft]

I like this, but want to propose a slightly different API.

Something like this would satisfy @marcoroth's request for supporting return values on all morph modes, but without changing the args passed to the broadcaster.

  // stimulus controller
  ajax: (search, callback) => {
    this.stimulate('Example#lookup', search).then(payload => callback(payload.reflexReturnValue))
  }

  # reflex
  def lookup(search)
    self.return_value = [{text: "chris", value: 1}, {text: "chip", value: 2}]
    morph :nothing
  end

It's a bit more explicit; albeit, a little less elegant, but it also supports more use cases. I think this is justified given that reflex return values are something of an edge case, but one I'd like to see supported across all morph modes. Thoughts?

[leastbad]

I actually love your suggestions, Nate.

My primary hestitation for doing just Nothing was not wanting to refactor Page/Selector morphs to handle an extra event operation that would need to be dispatched, just on the off-chance that someone would want to use a return value. Now that I've had some time/distance, I think we could expand what we do here and here to make it work for all three.

I'm a little fuzzy on how to implement your suggested self.return_value syntax. Is this a cattr_writer? If you are able to go into a bit more detail on how you'd write this one, I'm sure I'll be able to run with it.

Thanks for the review!

[hopsoft]

I was thinking of something along these lines.

class StimulusReflex::Reflex
  attr_accessor :return_value
  ...
end

class Broadcaster
  ...
  def broadcast_message(subject:, body: nil, data: {}, error: nil)
    ...
    cable_ready.dispatch_event(
      name: "stimulus-reflex:server-message",
      detail: {
        reflexId: data["reflexId"],
        reflexReturnValue: reflex.return_value,
        ...
  end
end

We'd then use a similar technique on the page and selector broadcasters.

class PageBroadcaster < Broadcaster
  def broadcast(selectors, data)
    ...
    cable_ready.morph(
      selector: selector,
      html: html,
      reflexReturnValue: reflex.return_value,
      ...
  end
end

[julianrubisch]

I love the idea of having return values everywhere.

My current use case: I tack a UUID on temporary <div>s to appease morphdom, which I want to remove later. with return values this could be as easy as

this.stimulate(...).then(({ uuid }) => this.element.querySelector(`[data-uuid=${uuid}]`).remove() })

[leastbad]

Alright @julianrubisch @marcoroth, I think this should make you happy!

@hopsoft I noticed that the selector broadcaster tests were all commented out, but I updated them anyhow.

This implementation is so much simpler.

I updated the description with the current syntax and details (eg. no longer just Nothing Morphs).

[hopsoft]

This looks great.

@marcoroth did have some good feedback in Discord on this PR. Namely that perhaps self.return_value doesn't feel as Railsy as it possibly could. I think this is a valid observation.

What you're doing in this PR is somewhat similar to output parameters in stored procedures. What if we changed it to use an object that feels similar to Rails flash but named it output?

output[:value] = 123
output[:another_key] = "why not, it's cool"

The entire output Object/Hash would be what's emitted as detail.output on client the side.

The actual implementation wouldn't need to change much to support this. Thoughts?

[julianrubisch]

Okay, since we're having this discussion, I think payload would transmit the intent best.

output is a somewhat too generic name, in my ears

[marcoroth]

I think payload would transmit the intent best.

Yes I think too, that's why I went with payload:

def lookup(search)
  payload[:value] = [{text: "chris", value: 1}, {text: "chip", value: 2}]
  payload[:and_another_one] = "because why not"

  morph :nothing
end

[hopsoft]

I'm not stuck on output, so payload seems like a reasonable choice too.

[leastbad]

I've noticed that you do need to preface the accessor (whatever it's called) with self. though I admit I'm not actually sure why this is necessary. I just tried removing the self. and it just treats it as me declaring a local scope variable.

---

I actually don't love payload for this usage, even though it was part of the example I wrote. My original suggestion was returns which I liked a lot. Another suggestion could be emits.

Does making the accessor a hash automatically make the solution feel more Railsy? Surely this is a little reductive...

I am not planning to waste mana blocking such a niche thing, but it really does seem like if someone wants to send multiple values, they could just send an array or a hash instead of forcing everyone to assign a key even for simple values.

[leastbad]

Okay, I have updated the code to return a Hash called payload that can be called using Symbol keys. Lemme know what you think.