futurism icon indicating copy to clipboard operation
futurism copied to clipboard

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

Open xtr3me opened this issue 3 years ago • 2 comments

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)

xtr3me avatar Nov 23 '21 10:11 xtr3me

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)

julianrubisch avatar Nov 23 '21 10:11 julianrubisch

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 avatar Nov 25 '21 10:11 xtr3me