Draft: Listen to cable-ready:after-update events and re-request the partial #120

xtr3me · 2021-11-23T10:41:01Z

Type of PR

feature / enhancement

Description

By listening to cable-ready:after-update events we are able to re-request the partial from the server. Allowing the server to serve the original content (without the need for unless) and stay fast.

Why should this be added

The server can serve it's original response without needing to include the futurismed partials which can potentially be slow to render. (That is probably the reason why futurism was chosen, at least it is in my case).

Checklist

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

This is currently a proposal / draft. I still need to try/test this locally and remove the found caveats.
I would like to get feedback on the proposed code in order to improve it / check if this would be the correct way to implement this enhancement.

Potential caveats are:

CableReady morphs the inner part of the futurism element, causing it to show the temporary loader div.
The listener is potentially added multiple times on the element, causing it to request the partial multiple times (is covered by the debouncer, but I would like to avoid this)
Shall we make this an opt-out enhancement or do we want to add this as an opt-in? (adding a data-attribute to the element for example in order to opt out/in)

julianrubisch · 2021-11-23T10:48:15Z

I'm currently pondering whether this should be opt-in. Honestly we have a long list of optional attributes already so I'm rather leaning on making this default behavior, albeit we should then probably remember to make CR >= 5 a hard dependency (which it is going to be anyway)

xtr3me · 2021-11-25T10:00:13Z

I've updated the PR and tried the changes locally. This solution works but it has a drawback:

Works only for non li & tr elements (due to the fact we wrap the element with an updates-for element.

The following things still need to be checked / altered:

Restoring the placeholder on turbo:before-cache events is not checked and i'm doubting it is compatible
Naming things is notoriously hard, this PR currently suffers from bad naming especially in the helper for example: wrap_for_updates_for & wrapped_for_updates_for
What happens when a developer has a form or other browser state (opened tab) in the partial, it currently will be reset. Do we want to fix this or do we just document the current behavior.
I would like to move the WrappingFuturismElement class to its own file as it is now also used within Futurism::Resolver::Resources
Currently I copied the contents of CableReadyHelper from CableReady as I was unable to include it. I would like to propose we place the methods of CableReadyHelper in a module and include that in the CableReady helper and include it in the WrappingFuturismElement class. (This is a change within CableReady)
Add more tests to validate this functionality
Check if this works correctly with the futurism eager loading feature

See the stimulusreflex/cable_ready#166 PR for the changes needed within the CableReady updates-for HTML element.

xtr3me · 2021-11-25T10:02:00Z

javascript/futurism_channel.js

@@ -16,6 +16,10 @@ const debounceEvents = (callback, delay = 20) => {
  }
 }

+const targetResolver = (event) => {


I have to check if this is still needed or a relic of my try & fail & try again approach

xtr3me · 2021-11-25T10:02:32Z

javascript/package.json

@@ -22,7 +22,7 @@
  },
  "homepage": "https://github.com/stimulusreflex/futurism#readme",
  "dependencies": {
-    "cable_ready": "5.0.0-pre8"
+    "cable_ready": "file:../../cable_ready"


I need to restore this

…ement to not morph but instead emit an event

xtr3me · 2021-11-25T10:04:14Z

lib/futurism/helpers.rb

        @html_options = options.delete(:html_options) || {}
        @data_attributes = html_options.fetch(:data, {}).except(:sgid, :signed_params)
        @model = options.delete(:model)
+        @wrapped_for_updates_for = options.delete(:wrapped_for_updates_for)
+        if @wrapped_for_updates_for


Not really happy with this variable name, any suggestions?

xtr3me · 2021-11-25T10:05:32Z

lib/futurism/helpers.rb

        @html_options = options.delete(:html_options) || {}
        @data_attributes = html_options.fetch(:data, {}).except(:sgid, :signed_params)
        @model = options.delete(:model)
+        @wrapped_for_updates_for = options.delete(:wrapped_for_updates_for)
+        if @wrapped_for_updates_for
+          @html_options[:keep] = 'keep'


Needed to not add the IntersectionObserver (as it will re-request the partial over and over again when it is visible)

xtr3me · 2021-11-25T10:06:56Z

lib/futurism/helpers.rb

+        @wrapped_for_updates_for = options.delete(:wrapped_for_updates_for)
+        if @wrapped_for_updates_for
+          @html_options[:keep] = 'keep'
+          @data_attributes['updates-for'] = true


Instructing the javascript to listen for the CableReady cable-ready:after-update event. As we only want to refresh the partial if it has been rendered before.

xtr3me · 2021-11-25T10:07:30Z

lib/futurism/helpers.rb

+      private
+
+      ############
+      # TODO: Include CableReadyHelper


Remove this and include it from CableReady once it's moved to a module.

xtr3me · 2021-11-25T10:08:10Z

lib/futurism/resolver/resources.rb


          yield(resource_definition.selector, html, resource_definition.broadcast_each)
        end
      end

      private

+      def wrapped_html(html, options)


Name of this method can be improved

Listen to cable-ready:after-update events and re-request the partial

03ad0c6

xtr3me changed the title ~~Listen to cable-ready:after-update events and re-request the partial~~ Draft: Listen to cable-ready:after-update events and re-request the partial Nov 23, 2021

julianrubisch marked this pull request as draft November 23, 2021 13:09

xtr3me mentioned this pull request Nov 25, 2021

Draft: Futurism updates-for support stimulusreflex/cable_ready#166

Closed

xtr3me commented Nov 25, 2021

View reviewed changes

Wrap futurism with CableReady#updates_for and instruct updates-for el…

78b5bef

…ement to not morph but instead emit an event

xtr3me force-pushed the add_cable_ready_after_update_listener branch from c8ddc14 to 78b5bef Compare November 25, 2021 10:03

xtr3me commented Nov 25, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Listen to cable-ready:after-update events and re-request the partial #120

Draft: Listen to cable-ready:after-update events and re-request the partial #120

xtr3me commented Nov 23, 2021

julianrubisch commented Nov 23, 2021

xtr3me commented Nov 25, 2021 •

edited

Loading

xtr3me Nov 25, 2021

xtr3me Nov 25, 2021

xtr3me Nov 25, 2021

xtr3me Nov 25, 2021

xtr3me Nov 25, 2021

xtr3me Nov 25, 2021

xtr3me Nov 25, 2021

Draft: Listen to cable-ready:after-update events and re-request the partial #120

Are you sure you want to change the base?

Draft: Listen to cable-ready:after-update events and re-request the partial #120

Conversation

xtr3me commented Nov 23, 2021

Type of PR

Description

Why should this be added

Checklist

julianrubisch commented Nov 23, 2021

xtr3me commented Nov 25, 2021 • edited Loading

xtr3me Nov 25, 2021

Choose a reason for hiding this comment

xtr3me Nov 25, 2021

Choose a reason for hiding this comment

xtr3me Nov 25, 2021

Choose a reason for hiding this comment

xtr3me Nov 25, 2021

Choose a reason for hiding this comment

xtr3me Nov 25, 2021

Choose a reason for hiding this comment

xtr3me Nov 25, 2021

Choose a reason for hiding this comment

xtr3me Nov 25, 2021

Choose a reason for hiding this comment

xtr3me commented Nov 25, 2021 •

edited

Loading