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

Scoped Stimulus Reflex controllers #43

Merged
merged 35 commits into from
Oct 5, 2019

Conversation

leastbad
Copy link
Contributor

@leastbad leastbad commented Sep 17, 2019

Feature Proposal: many atomic Reflex instances living happily together

Description

I have implemented my proposed API and technique to allow many scoped SR widgets to coexist in the same document, alongside non-Reflex elements. It requires a few significant changes to the way things are done and admittedly is a bit of a bull in the china shop - it requires changes to cable_ready (a PR which doesn't break the existing API has been created already) as well as stimulus_reflex. However, I believe that the payoff is huge and that it's a solution which will adapt well to future innovations from the Rails team eg. ActionView::Component.

The biggest change - and the reason I had to patch cable_ready - is that I propose we switch from CSS selectors to XPath. I love selectors for web development but they are brittle in the context of what we want to do. I have added functionality to stimulus_reflex to access the XPath query for an element, and similarly cable_ready can now move from XPath back to an element.

The other significant effort in this PR pertains to the way we generate Stimulus controllers for every declarative Reflex in the DOM. At face value, this is fine - maybe even great - but there's an issue that keeps coming up, which is that the SR doesn't keep a reference back to the controller it's conceptually attached to. These controllers all show up as "stimulus-reflex" and technically they can call any Reflex module on the server that's available, even if there isn't a controller that matches the name of the Reflex in scope. It works but it leaves a lot of room for interpretation on the patterns that make sense for this style of development.

Well, I decided that I sort of hate this and that we probably have a short window to create a really strong convention about how all of these components should play with each other:

  1. All declarative Reflexes must have a corresponding controller defined, even if all they do is call StimulusReflex.register() in the connect(). Reflexes will now appear to have been called from their namespaced controller. This is because when we re-render this component...

  2. All controllers in use must be assigned to at least one DOM element, which becomes the root node which is replaced by MorphDom. There needs to be a 1:1 relationship between the DOM element that has the data-controller attribute and where cable_ready will deliver the HTML payload.

Let's consider the following example:

<div data-controller="pizzas">
  <button data-reflex="click->PizzasReflex#pizza">Order Pizza</button>
</div>

If we accept my changes, this button will be managed by a "stimulus-reflex" controller, but it will appear to be part of the Pizzas controller. stimulus_reflex handles declarative and explicit stimulate calls differently to make this possible. We use the Stimulus getControllerForElementAndIdentifier function to gain a reference to the Pizzas controller, and generate an XPath query for that controller's DOM element. When the content is updated, it is the div and its children that gets updated by MorphDom.

The biggest change to the SR backend is that we now have to send an XPath query as the 5th parameter on each call to the server. SR's Ruby module uses this to both feed it into Nokogiri (which is likely way faster with XPath) as well as to pass along the appropriate subset of the document to cable_ready. To be clear, the full DOM is still being rendered (give me a few more weeks! 🤓 ) but it is not delivered to the client.

I have put a lot of work into setting up a solid todomvc example that has 3 different SR contexts running simultaneously alongside a simple DOM textbox element that is not touched by SR, as a means of showing what's possible. That said, in order to try it out, you pretty much have to be prepared to pull my PRs to stimulus_reflex, cable_ready and stimulus_reflex_todomvc. You have to point the stimulus_reflex gem to a local path, and you have to make sure that you yarn link both cable_ready and stimulus_reflex so that webpack will use the right packages. Finally, you have to make sure that you run rails dev:cache once before my demo will work.

hopsoft/stimulus_reflex_todomvc#21

As usual, I am happy (thrilled, even) to do demos via Zoom for anyone interested.

Fixes # #26

Why should this be added

I share the excitement for @effkay's work on ActionView::Component integration but after significant reflection, I have several reasons for advocating a different way forward. I feel tremendous gratitude for Felipe's inspiration.

It's true that I'm way too impatient to wait for 6.1 to come out, but even if it was here today I strongly believe that this functionality should not be tied to it. Instead, let's come up with a smart way to accommodate both the traditional layout and nested component workflows. The more I build with Stimulus Reflex, the more I realize that scoped controllers are a) going to be used by pretty much everyone once they are available and b) the important atomic unit is the client-side widget, not the server-rendered code that powers it.

If you think about the original use case for React at Facebook, they wanted a way to be able to sync up all of the likes, comments and notifications against a stream. They aren't redrawing the whole interface.

Checklist

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

reflex

@hopsoft
Copy link
Contributor

hopsoft commented Sep 18, 2019

I would love a demo and walkthrough. There is some nuance here that I think we could flush out in conversation.

For example, I think it's a bit aggressive to expect that each reflex update should be restricted to the corresponding Stimulus element.

I have a sophisticated project that these libs were extracted from that uses small reflexes to update both Redis and Postgres. These small data updates impact several disparate sections of the page... and they all update because the entire body is rendered, DOM diff'd, and updated.

Having said that, I'd love to add the option for reflex updates restricted to the corresponding Stimulus controller.

@leastbad
Copy link
Contributor Author

Conversation: yes.

Deeply sensitive to extraction concerns.

Big picture: just throw a controller on your body?

@hopsoft
Copy link
Contributor

hopsoft commented Sep 18, 2019

just throw a controller on your body?

Not feasible for the use case. The controllers are also standard Stimulus controllers that do many other things. They are very much akin to React style components... they do things like expand/colapse, add/remove style, mutate hidden form values, etc... depending on what the user is doing. Some of those interactions trigger a stimulus reflex... which in turn ensures that all the disparate sections of the page get updated.

Hoisting all of those concerns up to the body would require a new architecture... that I'm fairly certain would be less compelling.

@hopsoft
Copy link
Contributor

hopsoft commented Sep 18, 2019

The principal idea being that focused isolated effort on a single HTML/Stimulus component can trigger backend data store changes with the confidence that the "state" changes will be reflected everywhere... automatically.

@pablo-co
Copy link

pablo-co commented Sep 26, 2019

This problem is what is blocking my team from adopting stimulus_reflex. In our case, we have a combination of server state and client state (StimulusJS). Client state gets reset every time StimulusReflex is involved which was a nightmare to patch when trying to adopt this library.

@hopsoft
Copy link
Contributor

hopsoft commented Sep 27, 2019

This is great feedback. I agree with high level goals of scoping DOM updates and lifecycle callbacks and believe that this feature (in some form) needs to find its way into the library.

I'm waiting for #45 to land before focusing too much on this feature though.

@leastbad
Copy link
Contributor Author

Lots of progress since this PR was submitted. Work on the new callbacks API is just about to wrap up ( #45 ) and with it comes some changes that require this PR to be reworked.

Specifically, you can now set a declared reflex on an element without writing any JavaScript! When you import StimulusReflex, it brings the magic. Even crazier: you can declare a reflex without marking any Stimulus controllers in your markup! These are probably not every-day use cases, but it means that we cannot assume that there will be a controller in play. We also cannot assume that there will be just one instance of a controller in your document.

My PR was based on some assumptions: there would be a single Stimulus controller for each Reflex class. This controller would be assigned to the parent element you wanted to replace. If you want to replace your whole body, you'd put a Stimulus controller on your body element. A reflex for "click->TodosReflex#edit" on any element would replace the DOM hierarchy starting at the element with that data attribute.

When I put these two realities together, I need to rework how scoping can work.

First thing first: body will always be the default point of replacement. You will never have to indicate that you want your app to refresh from the document root.

The new callback code means we welcome multiple Stimulus controller instances. When you trigger a reflex, it first looks to the element the declaration is on to see if it's also a controller that matches the reflex. If not, it walks up the tree until it finds an ancestor that is an instance of the appropriate controller.

My first idea on how this could work is to set an additional data attribute on an element that contains a data-controller you intend to be the root element for a scoped refresh. For example:

<div data-controller="todos" data-morph-root>
  <button data-reflex="click->TodosReflex#create">Create</button>
</div>

What I like about this is that you could have multiple instances of todos on your page and each would only update itself. No other instances would be affected. The button is a child of the div with the controller, so it knows which element to replace.

Meanwhile, if you have any reflexes in your UI that aren't themselves scoped to their parent controller, or the controller isn't marked as a morph root, then the entire document body would be refreshed. Sometimes this isn't a bad thing, but if you don't want your whole body to refresh then you'd need to make sure that all controllers are marked as morph roots.

Now, what this proposal doesn't cover is a mechanism for any given controller (or reflex action!) to override the element to be replaced. You can't point it to an arbitrary controller/element or CSS selector. You can't give it an xpath expression. There's no way to determine this destination dynamically at runtime.

While the Cable Ready gem does now support xpath expressions, I am currently of the opinion that allowing for dynamic or arbitrarily specified destinations is something we should steer away from. Not only does it get complex in a hurry with lots more edge cases, but I feel like it adds a ton of complexity to the API surface. It's a bit of a mind-bender when you want to update the area that's outside and above the controller calling it.

Basically, if you feel like setting morph roots all over your UI from inside of a controller is stifling then you should just stick to the default body.

Does this seem like an approach worth pursuing? If so, I can update my PR to work as I've described. @pablo-co

@hopsoft
Copy link
Contributor

hopsoft commented Sep 28, 2019

My first idea on how this could work is to set an additional data attribute on an element that contains a data-controller you intend to be the root element for a scoped refresh. i.e. data-morph-root

I've had very similar thoughts regarding scoped updates. I'm not sure about the data attribute name yet. Morph is really just an implementation detail. Perhaps something like data-dom-update might work better. 🤷‍♂


I'm also curious about possibly supporting a selector so a reflex could update a different part of the DOM.

<span id="counter"><%= @count %></span>
<div data-controller="todos">
  <button data-reflex="click->ExampleReflex#increment"data-dom-update="#counter">
    Increment
  </button>
</div>

@leastbad
Copy link
Contributor Author

leastbad commented Sep 28, 2019

I'm not attached to the name. Just a suggestion. I don't love dom-update but the only real requirement is that it be something that won't conflict with people's application domain naming conventions.

What I'm looking for is a go-forward with this approach in general. Once I get the "boolean" version up and running, adding the ability to target a Stimulus controller instance, a CSS selector or an XPath expression via string value are all incremental updates.

One significant difference between what I'm proposing and what (I think) you're proposing is that I'm suggesting the target element for update either be the element that has the data-controller attribute on it (strongly preferred) or whatever CSS selector is passed as a value. You're giving the triggering reflex element the ability to decide where the update goes.

The pro to your approach is certainly flexibility. The cons are that you have a lot of repetitive data-dom-update attributes on every reflex element and you give up the opportunity to have the controller neatly encapsulate the logic into what the kids refer to as a component.

@hopsoft
Copy link
Contributor

hopsoft commented Sep 28, 2019

I see this as a feature that the library definitely needs to support. Also, we are pretty close in our thinking about what the feature should be. Please take a stab and let's test it out.

@hopsoft
Copy link
Contributor

hopsoft commented Sep 28, 2019

I'm not convinced that our proposals are mutually exclusive. We could allow both the outer data-controller element and the more specific data-reflex element set a value for this new attribute. The most specific case would win. The order of precedence would be:

  1. data-reflex - whatever the selector points to
  2. data-controller - whatever the selector points to
  3. body - default

@leastbad
Copy link
Contributor Author

Just wanted to share my progress so far, which is that this should be a drop-in replacement for what's on master. Behind the scenes, it's now sending an XPath expression to the server. Right now, it's hard-coded to the document body.

If you are set up to easily swap your gem and js via rake install && yarn link and have a few minutes, I'd appreciate any early sanity checks.

@leastbad
Copy link
Contributor Author

reflex

One oddity I noticed is that when I dump the data object to the console log (inside of the stimulate() function) is that the data-action and data-controller attributes on those stimulus-reflex controllers are prepended with spaces.

It doesn't seem to be hurting anything, but it can't be right, either.

@leastbad
Copy link
Contributor Author

leastbad commented Sep 30, 2019

Alright, I think I actually have it "working" for a few of the cases. I took your preference priority and extrapolated a little bit:

  1. data-reflex-root (no value) == use element calling the reflex
  2. data-reflex-root=".todoapp" == lookup CSS selector and use it
  3. (on controller) data-reflex-root (no value) == use controller's element
  4. (on controller) data-reflex-root=".todoapp" == lookup CSS selector and use it
  5. document body

For cases 2 and 4 I fall back to using the reflex or controller's elements, respectively. Please let me know if you'd prefer I fall back to document body, or even raise an error if there is no selector match.

The good news is that cases 1, 3, and 5 have the desired effect right now, and I promise that it's just a coincidence that this is my favourite configuration.

2 and 4 appear to be working, but I realized just now that to support these cases, I have to further modify both the client and server libraries to support an additional XPath: one to process and one to mark the ultimate destination.

I absolutely can do this, with some caveats. For example, I'm pretty sure that MorphDom requires us to replace elements 1:1 with the same NODE_TYPE, especially given that we're flagging it to just swap the children. You can't take the contents of a div and diff it with a section. This would have to be explained in bold in the documentation or else people will be frustrated.

However, aside from preferring not to pollute the API(s) with a 2nd XPath expression, the biggest concern I have is how weird it seems to actually do this thing with the CSS selector. Can you give us a use case so I can wrap my brain around it? I genuinely do want to see what you see.

Just to make a small effort to convince you that the CSS selector thing is unnecessary, right now the following example should work just fine:

<div data-controller="foo" data-reflex-root>Go ahead and update me!</div>

<div data-controller="bar">
  <button data-reflex="click->FooReflex#update">Update Just Foo</button>
  <button data-reflex="click->BarReflex#update">Update everything in body</button>
</div>

Did I misinterpret your suggestion? Have a think and please advise.

@pablo-co
Copy link

Our team currently uses Turbograft for all dynamic and embedded interactions. For each unit of separate functionality we have a controller (which maps directly to a Reflex here) and we use several data-tg-* attributes to indicate turbograft what things to replace in the DOM and when. We are currently using Turbograft in around 8 different projects, ranging from a live trivia game to an internal Bank CRM. We are yet to find a use case that wasn't covered by their attribute/JS API.

In the weekend I can look into what an equivalent stimulus_reflex implementation would look like for one of our projects if that would help.

@leastbad
Copy link
Contributor Author

Thanks for the link and explanation, @pablo-co! This is extremely helpful for me as I use Turbolinks but was not familiar with Turbograft. I can't promise that the implementation we settle on will cover all of your use cases, but your input will inform how we proceed.

No question that using ActionCable as a transport medium is a desirable upgrade over Ajax requests. With a persistent open connection, it means that you can actually pass multiple messages down to the client and avoid having to wait on long-running operations. Updates don't have to be initiated by the client.

One thing I notice right away is that this library is a few years old and understandably not making use of MorphDom, which is highly performant. This suggests that behind the scenes it's probably "a candidate for welcome performance improvements". 😉

Phoenix LiveView, which significantly influenced the creation of Reflex, has a concept similar to tg-refresh-never. I have been exploring how this kind of functionality could work in Reflex. It's all but impossible when you're updating from the body, which is why I'm looking into scoping: to give us power and flexibility in how we define our applications.

I do see some things I don't like in Turbograft. The README suggests inlining a substantial amount of Javascript to manage what Turbolinks makes almost transparent. I have a question for you as an experienced Turbograft user: am I right is saying that the library asks the developer to choose between two design outlooks?

  1. Update nothing except for the elements you list
  2. Update everything except for the elements you mark as static

If true, then which conceptual outlook do you find that your team uses?

@hopsoft
Copy link
Contributor

hopsoft commented Oct 1, 2019

I really like the idea of supporting an attribute like tg-refresh-never.

@leastbad
Copy link
Contributor Author

leastbad commented Oct 1, 2019

LiveView has four distinct operations possible on an action:

  • replace - the default operation. Replaces the element with the contents
  • ignore - ignores updates the DOM regardless of new content changes
  • append - append the new DOM contents instead of replacing
  • prepend - prepend the new DOM contents instead of replacing

Right now we only support replacing the element's children. I suspect that append and prepend are quite useful for newsfeeds and chat streams.

The implementation for ignore in LiveView is here:

https://github.com/phoenixframework/phoenix_live_view/blob/e4c6f7140d5b8c383e584671a63c14e85e9c26e9/assets/js/phoenix_live_view.js#L756

Line 758 is particularly telling.

The implementation for append/prepend I haven't dug into as deeply, but in their implementation they require a target selector to append on to.

@leastbad
Copy link
Contributor Author

leastbad commented Oct 1, 2019

Okay, now we're cooking with gas. This commit should make @pablo-co very happy.

I had to pull out the XPath stuff because I don't have good answers for what happens if the result of a reflex action introduces side effects (ie adds or removes DOM elements that change the XPath expression).

As promised, the data-reflex-root attribute now accepts a comma-separated list of CSS selectors. These selectors are verified as returning a valid element before being sent to the server.

Now there are three ways to scope, down from five:

  1. data-reflex-root (no value) == use element calling the reflex
  2. data-reflex-root=".todoapp" == lookup CSS selector and use it
  3. (on controller) data-reflex-root (no value) == use controller's element
  4. (on controller) data-reflex-root=".todoapp" == lookup CSS selector and use it
  5. document body

In other words, to use this feature you must specify a value to the attribute. This is an about-face from my previous effort.

I haven't had time to publish a branch to the todomvc for you to test, but it should be very straight-forward:

todos_reflex.rb

  def words
    @words = element[:value]
    puts @words
  end

index.html.erb

<input type="text" value="<%= @words %>" data-reflex-root="[forward],[backward]" data-reflex="keyup->TodosReflex#words">
<section class="todoapp" forward><%= @words || rand(1000) %></section>
<section class="todoapp" backward><%= @words&.reverse || rand(1000) %></section>

I'm using non-data attributes a) because I can and b) because they won't get picked up as data attributes by Stimulus Reflex. Not sure this is good/bad/moot, but some explanation was in order.

Now, before I :shipit: there is one issue that is actually has me pretty stumped, and I would appreciate some more neuroplastic brains on it.

For some reason, when you tell morphdom to morph an element against some html in "children_only = true" mode, it creates a clone of the top-level wrapping element. This does not happen when morphing the document body. However, if you tell morphdom to children_only = false, it works but it kills all of the events you had registered.

This only happens one time, because querySelector is only going to take the first matching element. It's not breaking anything in my simple examples, but I'm sure it could introduce weird CSS issues for people using themes and frameworks that expect a specific hierarchy. It's also just clearly wrong, and that's annoying.

I've tried looking for ways to strip out the redundant wrapping tag; Nathan might remember that very early on in my involvement with the project, I had to write an ugly hack to negate this exact same issue. I essentially re-implemented jQuery's unwrap function. Bottom line, it's clearly either something I'm missing or a bug in morphdom or cable_ready. I'd like to get this sorted.

Anyhow, if you can work around or live with the duplicate element, this is technically ready to experiment with today.

@hopsoft hopsoft added enhancement New feature or request and removed proposal labels Oct 5, 2019
@hopsoft hopsoft merged commit 16d5978 into stimulusreflex:master Oct 5, 2019
@leastbad leastbad deleted the selectors branch October 5, 2019 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants