Declarative Reflexes Resiliency

[marcoroth]

Type of PR

Improvement

Description

This Pull Requests fixes two problems:
a) the double-fire of reflexes when invoking the reflex once
b) StimulusReflex not executing the client-side Stimulus controller callbacks when invoking a reflex

Because of load order reasons (which are mainly just apparent in Import Maps powered apps) setupDeclarativeReflexes() isn't always run at right time. In browsers, where the Import Maps are not natively supported yet, it leads to the fact that the setupDeclarativeReflexes() is ran twice. Since there is also no guarantee of load order we can't assume that all Stimulus controller instances are loaded yet.

In some instances this had weird side effects. If you have this element:

<button 
  data-reflex="click->Example#create" 
  data-controller="example"
>Create</button>

it would convert the element to this the after calling setupDeclarativeReflexes() the first time around, because the example controller instance is not setup yet:

<button 
  data-reflex="click->Example#create" 
- data-controller="example"
+ data-controller="example stimulus-reflex" 
+ data-action="click->stimulus-reflex#__perform"
>Create</button>

and after calling setupDeclarativeReflexes() the second time with the example controller now available:

<button 
  data-reflex="click->Example#create" 
  data-controller="example stimulus-reflex" 
- data-action="click->stimulus-reflex#__perform"
+ data-action="click->stimulus-reflex#__perform click->example#__perform"
>Create</button>

The fact why this issue appeared in the first place is because the setupDeclarativeReflexes() is looking if the developer defined a data-controller on the element, and if so we are using that controller so we can invoke the callbacks in that controller.

But at the time when the setupDeclarativeReflexes() function is being run the first time the example controller instance isn't connected yet and our function then opts for setting the generic stimulus-reflex controller instead. At the second time the example controller instance is loaded and it will append the second action descriptor, because that one wasn't present already so it causes the action descriptor to be set twice causing the double-fire issue.

Because there are now two action descriptors setup on that element it's also going to invoke the reflex twice. This Pull Requests refactors the setupDeclarativeReflexes() function and makes it aware of previously set action descriptors so it can replace them instead of just appending them to prevent the double-fire issue.

I guess depending on the browser used and the browser version it would not add the action descriptor twice, but instead would just use the wrong Stimulus controller and therefore not invoke the Stimulus controller callbacks.

In order to fix that, we extract a function setupDeclarativeReflexesForElement(element) which can be called with a single element to setup the declarative reflexes on it. This function is being called when the Stimulus controller calls StimulusReflex.register(this), which then setups the right action descriptors, guaranteeing that the right controller is actually present.

Because of this approach, we might even be able to remove the global event listener for setting up the declarative reflexes on document:

stimulus_reflex/javascript/stimulus_reflex.js

Lines 240 to 244 in cb62c08

	document.addEventListener('readystatechange', () => {
	if (document.readyState === 'complete') {
	setupDeclarativeReflexes()
	}
	})

Fixes #605
Follow-up of #602

Why should this be added

Ensures that the data-reflex attribute gets properly converted to it's corresponding data-controller and data-action attributes by making the conversion independent of the events dispatched on window or document, but rather by hooking into the StimulusReflex.register(this) function which anyway just gets called when the Stimulus controller gets connected, meaning that the right Stimulus controller instance will always be available.

Checklist

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

[leastbad]

I generally like how you've approached this, and agree that it's smart to have register call a per-element version of this function.

Are you confident that filtering controllers on !action.includes("#__perform") will properly handle scenarios where a single element could have Reflex actions declared from multiple classes? For example input->foo#__perform click->bar#__perform.

If we get rid of the global setupDeclarativeReflexes, we need another place to call emitEvent('stimulus-reflex:ready'). Indeed, it's not clear that we even confidently know that more controllers won't load in.

[marcoroth]

Are you confident that filtering controllers on !action.includes("#__perform") will properly handle scenarios where a single element could have Reflex actions declared from multiple classes? For example input->foo#__perform click->bar#__perform.

Yeah, that should be covered, it might be, that we are invoking the function twice for such an element, but that shouldn't matter now since it should be resilient enough.

[marcoroth]

We might be able to emit the StimulusReflex ready event after the initialize function finshed or after the ActionCable connection established, since we don't need to "care" about the controllers in that case

[leastbad]

We might be able to emit the StimulusReflex ready event after the initialize function finshed or after the ActionCable connection established, since we don't need to "care" about the controllers in that case

The reason that event gets emitted is to tell the developer that it's ready to process events. If we emit before the controllers have registered themselves, then the event will come before the handlers are installed. This was fine in a syncronous config...

Otherwise, if you're confident about multiple classes working fine (should we add a test for this?) LGTM.

[marcoroth]

Okay I kept the global event listener with the setupDeclarativeReflexes() so that we still have a stimulus-reflex:ready being emitted with the best guess we have available.

I also added some tests for setupDeclarativeReflexesForElement().

[leastbad]

Working through this, I'm now of the opinion that stimulus-reflex:ready should only be called once per hard refresh, but I don't think that this is the correct place to enforce this.