Morph Modes: page, selector and nothing

[leastbad]

Description

This PR adds the concept of Morph Modes to StimulusReflex; Morph Selector, Morph Nothing and the current behavior, Morph Page.

Thanks to the thoughtful design of the existing library on both the client and server, it was possible to bolt on two entirely new modes of operation without any dramatic changes to existing functionality. Both benefit from the same logging, callbacks, events and promises as the classic refresh mode.

I suspect that a strict interpretation of SemVer will force a minor version bump.

Selector Morphs are similar to classic Page Morph operations, except that they do not call ActionDispatch - the controller is not run and the view template is not re-rendered. Instead, the developer can call the morph method one or more times to push an arbitrary string to an arbitrary DOM element. Selector morphs are syncronous - allowing you to morph a selector to display a spinner, call a slow API, and then remove the spinner... all in the same morph. Selector morphs also accept a hash syntax, allowing multiple morphs to be executed asyncronously in the same CableReady broadcast.

Yes, not hitting the routing engine OR ActionDispatch means that you're going to be seeing a lot of 0-1ms updates. Yes, this means that you can return a rendered partial collection, a ViewComponent or a string.

Morph Nothing is for calling Reflex actions that initiate long-running processes like ActiveJobs or brewing your morning coffee. They are the perfect place to make CableReady broadcasts that surgically update tiny bits of your DOM, like message notifications. When a Morph Nothing Reflex completes, no morphing occurs but all of the usual suspects (callbacks etc) fire.

I have added an optional 2nd parameter to stimulate. It has to be an object that contains at least one of the following keys: attrs, reflexId, selectors. If this options parameter is passed, it is shift()-ed out of the arguments splat.

Yes, you can now supply your own reflexId, and even override the attrs if you really know what you're doing. selectors is supposed to be an array of strings; if you pass #poop it will quietly coerce it into ['#poop'] for you. In the next version of the library, data-reflex-morph-target will replace data-reflex-root (now deprecated).

Page reflexes that currently specify a data-reflex-root will get notified of both the switch to data-reflex-morph-target, and that morph_target will be ignored for Page reflexes starting with the next version.

Before I forget: I made the left/right arrows on @marcoroth's logging mechanism more defined. I'm getting old and my eyes were squinty trying to figure out whether I was looking at requests or responses.

Okay, demonstration time!

<div data-controller="evolve">
<div data-controller="evolve">
  <h4>Stimulus controller triggers</h4>
  <button type="button" data-action="evolve#page">Page</button>
  <button type="button" data-action="evolve#pageWithSelector">Page with Selector</button>
  <button type="button" data-action="evolve#selector">Selector</button>
  <button type="button" data-action="evolve#nothing">Nothing</button>

  <br><br>

  <h4>Declarative Reflex actions</h4>

  <button type="button" data-reflex="click->Evolve#page">Page</button>
  <button type="button" data-reflex="click->Evolve#selector">Selector</button>
  <button type="button" data-reflex="click->Evolve#nothing">Nothing</button>

  <br><br>

  <div id="poop">I'm sad <%= "and stinky" if @stimulus_reflex %></div>
  <div id="yuck">When will it end</div>

  <div id="pop"></div>
  <div id="mike"></div>

  <br><br>
  
  <div>A random number is: <%= rand(100) %></div>
</div>

import { Controller } from 'stimulus'
import StimulusReflex from 'stimulus_reflex'
import consumer from '../channels/consumer'
import CableReady from 'cable_ready'

export default class extends Controller {
  connect () {
    StimulusReflex.register(this)
    consumer.subscriptions.create('EvolveChannel', {
      received (data) {
        if (data.cableReady) CableReady.perform(data.operations)
      }
    })
  }

  page () {
    // 2nd argument is an optional object containing *at least one of*: reflexId, selectors, attrs
    this.stimulate('Evolve#page', { reflexId: 'covid' })
  }

  pageWithSelector () {
    // accepts a string or an array of selectors, now
    this.stimulate('Evolve#page', { selectors: '#poop' })
  }

  selector () {
    this.stimulate('Evolve#selector')
  }

  nothing () {
    this.stimulate('Evolve#nothing').then(payload =>
      console.log('Works with promises!', payload.morphMode)
    )
  }

  afterPage () {
    console.log('Page rendered!')
  }

  afterSelector () {
    console.log('Selector rendered!')
  }

  afterNothing () {
    console.log('Nothing rendered!')
  }
}

class EvolveReflex < StimulusReflex::Reflex
  include CableReady::Broadcaster
  def page
    puts "This is a classic SR full-page morph"
  end

  def selector
    puts "This morphs the target element"
    # ApplicationController.render(partial: "layouts/navbar/notification", locals: { notification: notification })
    # ApplicationController.render(ConfirmEmailComponent.new(user: current_user))
    morph "#poop", "I am full of it!" # default syntax is `morph "CSS selector", string` and it will be synchronous
    morph "#yuck" # string content defaults to an empty string; this will be a 2nd CableReady broadcast
    morph "#pop": "rocks", "#mike": "ike" # you can pass an implicit hash of multiple morphs that will be broadcast asynchronously
    # morph :nothing # this would raise an error because once you switch from page to selector or nothing, there's no going back
  end

  def nothing
    puts "This is where you initiate an ActiveJob or CableReady broadcast; it does not morph the client"
    morph :nothing
  end
end

Fixes #39

Why should this be added

Many newcomers request the ability to update sections of the page and/or updating from specified URLs that might differ from the current page. While we understand why they are asking for these tools, the design of SR was intentional and it doesn't actually make sense to update the current page with the rendered content of a different page. There are simpler, clearer ways that won't confuse TF out of users. Morph Selector Reflexes are the perfect solution to this legitimate request.

Also, as the Ruby community lights up with the possibilities of CableReady, a Morph Nothing reflex is the rug that really ties the room together - it will make it easy for people to kick off long-running processes and call CableReady broadcasts. It is the perfect corollary to CableReady, in that it is a command sent from the client to the server.

Checklist

• My code follows the style guidelines of this project
• Checks (StandardRB & Prettier-Standard) are passing

[marcoroth]

@leastbad Nice! Great work on this PR!
I really love the idea of modes and can see them become very useful! (I can already see where I can improve stuff in my exisiting apps 😉)

The only concern I have is, that we "clutter" the framework by introducing a lot of new and sometimes specific data attributes (not particular to the features you just proposed, but in general). Is it just me to be overcautious?

[julianrubisch]

I think @marcoroth is right that we have to watch very carefully how the API evolves. @leastbad‘s addition looks pretty straightforward in this respect.

The only thing that worries me a bit - and please take this with a grain of salt - I truly think we should start integration testing

[hopsoft]

I have a set of integration tests stood up against the expo site but it's not complete and we need to make such testing more accessible to all contributors.

[leastbad]

@marcoroth I agree with you 100% about being extremely careful about adding data attributes.

... says the guy intending to add mode - the first one since he added/championed permanent and root 🤠

[leastbad]

Just throwing it on the table: there is a reasonable and possibly desirable alternative to introducing a new data attribute, and that is to revisit the syntax and conventions of data-reflex.

We borrow the Stimulus -> for refresh.

What if >> indicated a launch? What if ~> indicated an update?

While we’re brainstorming, @hopsoft is there a dimension close to ours where you’d consider making the string fragment Reflex optional, as it is completely redundant? All declared calls point to a reflex class, and all reflex classes end in Reflex. We could lop off six characters from every declaration.

[marcoroth]

On my first read-through I didn't get that you put the data-reflex-mode attributes on the buttons and wondered how you decide which type of mode you are going to use. I first thought it's based on the reflex method names 🙃

I just had a quick thought. I know, those are technically 3 new data-attributes but would eliminate the need for data-reflex-mode.

<!-- data-reflex defaults to "refresh" -->
<button type="button" data-reflex="click->EvolveReflex#refresh">Refresh</button>
<button type="button" data-reflex-refresh="click->EvolveReflex#refresh">Refresh</button>
<button type="button" data-reflex-launch="click->EvolveReflex#launch">Launch</button>
<button type="button" data-reflex-update="click->EvolveReflex#update" data-reflex-root="#poop">Update</button>

This one has the disadvantage that you have 4 attributes which technically almost do the same thing. I don't know 🤷‍♂️

I personally don't know which style I'd prefer, but I just wanted to share it with you anyway 😉

[leastbad]

I also wanted to say, for posterity, that I’m not emotionally attached to refresh, update or launch as terms. I entertained many options, and these happened to float to the top.

It’s entirely likely that there’s a better word than launch, for example.

[leastbad]

Sorry @marcoroth. We’re big kids here, you know I love you. Hard pass on introducing three new attributes.

[websebdev]

Personally I think I prefer the data-reflex-mode attribute as it's more open if one day we wanna add more modes. And the behavior seems also more obvious than ~>, >>.

[marcoroth]

@leastbad I'm fine with this since I’m also not a big fan of introducing 3 new attributes (after I wrote that we should be careful about adding new ones 🙈)

I just wanted to share the idea anyway 😄

[leastbad]

@seb1441 good points!

Arguments for modifying data-reflex

• shorter syntax
• puts update and launch on equal footing with refresh (no extra markup required)
• no need to expand the schema

Arguments for adding data-reflex-mode

• more easily allows future expansion
• syntax is more obvious
• work is already done

[hopsoft]

I really like the ideas here and have been considering the public API for a while now. While I like how terse modifying data-reflex attribute values would be, it poses a higher risk of being confusing and prone to user error/misunderstanding (especially for developers new to the library). It also breaks with our convention of repurposing Stimulus semantics, so my vote is for data-reflex-mode.

Having said that, I'd like to propose a new name for data-reflex-mode because I think the current semantics on the PR are a bit too presumptive about the types of things people might use these features for. Here's my proposed re-naming scheme.

• data-reflex-mode=refresh to data-reflex-render=page default
• data-reflex-mode=update to data-reflex-render=partial
• data-reflex-mode=launch to data-reflex-render=none

I realize that using partial as a value is making some assumptions; however, I think it's acceptable because (1) if feels very much like Rails (2) the meaning serves a dual purpose, so even if you render a String or a ViewComponent, it's still a partial page update.

I'd love to hear what others think.

[julianrubisch]

Very thoughtful, 100% agree

[marcoroth]

I like the idea of renaming it to data-reflex-render. I just think it could be a little bit confusing to distinguish between data-reflex-render=partial and data-reflex-render=page with a data-reflex-root.

[hopsoft]

@marcoroth Makes a great point and his concerns make me wonder if we should consider deprecating data-reflex-root as part of this PR. It would force a major version bump, but I think removing similar/competing features from the library is worth the effort and potential pain. At the very least we should consider deprecating data-reflex-root in favor of data-reflex-render and notify users with deprecation warnings in the logs.

Thoughts?

[julianrubisch]

I‘d also opt for deprecating it for the moment

[marcoroth]

@hopsoft In my opinion we shouldn‘t deprecate data-reflex-root because it would remove some of the awesome magic StimulusReflex provides.

If I would need to provide the return value for every root in the Reflex it would be much more work then just letting StimulusReflex rerender the page through the controller and just let it morph the given DOM element(s).

[marcoroth]

After I wrote that I realized that we could also name it the way where the "response" is rendered. Either in the controller or in the reflex.

This would lead to:

• data-reflex-render="controller" default
• data-reflex-render="reflex"
• data-reflex-render="none"

---

But apart from the naming, we would still need the data-reflex-root to define what part/element on the page should be replaced with the return value of the reflex method.

[leastbad]

Of course I went to sleep at the exact moment everyone started saying interesting things...

Okay, thanks for thinking about this so much! I was reading the comments and either in complete agreement or totally willing to go along with the group when I got to the part about potentially deprecating data-reflex-root. This bewilders me to the degree that I feel like I must be missing something.

If you get rid of data-reflex-root then how do you tell it which section to update with data-reflex-render="partial"? It's not hyperbolic for me to say that the reason I wrote this entire PR is specifically to enable people combining these two tools, as I did in the example on the PR. render/partial + root is the money shot. I'm very excited about none/launch, but it comes on the coat-tails of a vision for people using this targetted partial pattern a lot.

If we're worried about a confusing combination of names, I have two alternative suggestions. This first, respectfully, is that this concern is almost certainly overblown as we have documentation, examples and group of users who have clearly expressed a desire for this feature to exist... and they can be taught a new thing. The keywords aren't particularly confusing IMHO. The second suggestion is that we could introduce data-reflex-target with identical functionality to data-reflex-root and deprecate data-reflex-root, removing it in v4.

The only other idea I have at the moment is that in terms of keywords, I like "component" roughly as much as "partial" because it speaks to what form the result will take. I feel like partial is a means to an end, while many folks think of atomic chunks as components. This isn't a deal-breaker either way - I'd be perfectly happy with partial, too.

[hopsoft]

I agree that removing the functionality is not what's desired; rather, more clarity in the API. I think what's not quite feeling right is supporting both:

1. data-reflex-render=page + data-reflex-root
2. data-reflex-render=partial + data-reflex-target
   I think target works better than root here semantically.

It feels like option 2 should be the preferred mechanic for partial page updates, so I'm questioning the value of keeping option 1 once this PR ships. Note that it's entirely possible that there are valid use cases, but I'd also prefer to minimize competing features in such a small library.

[hopsoft]

I'll also go on record in defense of keeping partial for data-reflex-render as I think it fits better with Rails and still works semantically for pretty much any rendering preference. Be it components, partials, or something else entirely.

[hopsoft]

To address @marcoroth's proposal of using controller, reflex, none instead of page, partial, none. I still prefer using the format of what vs where for expando=value.

[leastbad]

Thank you for explaining this so clearly. I completely agree; the existance of partial+target makes page+root feel awkward and superfluous.

I feel good about have a page update be a page update. Simplifies everything. People can use data-reflex-permanent to mark things like Google Analytics which must be protected.

Question, is there still room for accepting a list of selectors, or should we go 1:1 for an update? It seems like if they want to do more than 1:1, they should start planning to call CableReady directly, no?

[leastbad]

I'm going to update the PR with what I think are the latest decisions. Should I be changing data-reflex-root to data-reflex-target? How do we want to roll this out? v3?

[hopsoft]

Love this PR and conversation. I think letting it simmer for a month has proven incredibly helpful for the thinking and decision making.

Key decisions that I very much agree with.

1. Moving the rendering concern to the server
2. Moving DOM selectors to the server
3. Keeping data-reflex-root behavior as is
4. Keeping default behavior to re-render the page as is

API

I think that repurposing render semantics is problematic. i.e. different method signature, ok to invoke multiple times, etc... I'm liking morph as the method name and agree that it's descriptive and not really tied to morphdom. I'm not yet sold on the Hash argument given that we can call morph multiple times. We can always add it later if people really want it.

Usage

Here's how I see the API usage shaping up.

class ExampleReflex < ApplicationReflex
  def page_morph
    # page will automatically re-render, this is the default
  end

  def targeted_morph
    morph "#selector", ApplicationController.render(partial: "/path/to/partial")
  end 

  def multiple_targeted_morphs
    morph "#selector", ApplicationController.render(partial: "/path/to/partial")
    morph "#selector", ApplicationController.render(partial: "/path/to/partial")
    morph "#selector", ApplicationController.render(partial: "/path/to/partial")
  end

  def no_morph
    morph :nothing
  end
end

If I'm not mistaken, this will not introduce any breaking changes which means we can get away with a minor version bump.

[hopsoft]

Also, this PR resolves #186

[julianrubisch]

Once this is merged, I could pick up work on #183 again, if it is still desired.

Also, let me restate my question about semantic versioning. @hopsoft‘s API 👆🏻 has the benefit that it would not break existing implementations, IIUC?

[hopsoft]

@julianrubisch I'd love to sync and collaborate on your ideas around #183 once things settle down a bit for me.

[jonathan-s]

Also happy to sync on the equivalent javascript adapter once things have settled down a bit :)

…

Sent from my mobile

On Sat, 27 Jun 2020, 16:24 Nate Hopkins, ***@***.***> wrote: @julianrubisch <https://github.com/julianrubisch> I'd love to sync and collaborate on your ideas around #183 <#183> once things settle down a bit for me. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#211 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQGYEXE6MX6YHTFKS6UJZ3RYX6ILANCNFSM4NIK6D2Q> .

[leastbad]

I love where this landed; no need to specify a morph type unless it’s :nothing.

I’m going to get to work on making this exist. Once it’s done, let’s each of us try to kick the tires really hard for a week. I’m going to try and script a video to go out for the release.

[julianrubisch]

Okay I‘m being a know it all here, but I really have a strong feeling „morph_mode“ should be its own class, or rather superclass. That way you could resort to polymorphism here (and possibly at other locations too) and do „tell don‘t ask“

[leastbad]

Respect. So, here's what's happening: I'm merging from master, then pushing, then updating the readme, then going to sleep. I'd be thrilled to wake up to a PR.

Truth is, that sounds great but I have no idea how to do what you're suggesting. If you want it, I'd love to learn.

[hopsoft]

I'm liking where this last refactor is headed. Had some questions.

[leastbad]

Starts with a bang, ends with a whimper. 💯

[leastbad]

Thank you to @hopsoft @julianrubisch @marcoroth @seb1441 @joshleblanc @jonathan-s @ideasasylum @RolandStuder @mtomov and @davidalejandroaguilar for your opinions, reactions, advice and patience. Go team!