Don't choose first controller if no matching StimulusReflex-enabled c…

…ontroller was found (#670)

This pull request solves an edge-case for scenarios when StimulusReflex
tries to detect the right controller to use for performing the
StimulusReflex `__perform` controller action for executing reflexes.

For example, if an element had a `data-controller` and a `data-reflex`
attribute, and the controller in `data-controller` was a
StimulusReflex-enabled controller, but didn't match the Reflex specified
in the `data-reflex` there were some cases where it wouldn't add the
default `stimulus-reflex` controller to the element, but would still
reference the `stimulus-reflex` controller in the `data-action`
attribute. This basically lead to a no-op for the reflex action.

Like:
```html
<a
  data-controller="
    some-sr-controller
  "
  data-reflex="click->Example#doSomething"
></a>
```

was expanded to this in some cases:
```diff
<a
  data-controller="
    some-sr-controller
  "
  data-reflex="click->Example#doSomething"
+ data-action="click->stimulus-reflex#__perform"
></a>
```

Whereas it should have been:
```diff
<a
  data-controller="
    some-sr-controller
+   stimulus-reflex
  "
  data-reflex="click->Example#doSomething"
+ data-action="click->stimulus-reflex#__perform"
></a>
```
With this, the `data-action` is not referencing an undefined Stimulus
controller anymore and the reflex should execute again.

## Why should this be added

For consitancy reasons, so that the declarative reflex syntax still
continues to make sense and work as expected.

Thanks to @foliosus for helping to debug this!

javascript/controllers.js

@@ -43,7 +43,7 @@ const findControllerByReflexName = (reflexName, controllers) => {
     return identifier === controller.identifier
   })
 
-  return controller || controllers[0]
+  return controller
 }
 
-export { allReflexControllers, findControllerByReflexName }
+export { localReflexControllers, allReflexControllers, findControllerByReflexName }

javascript/scanner.js

@@ -42,7 +42,9 @@ const scanForReflexesOnElement = (element, controller = null) => {
 
     const parentControllerElement = element.closest(`[data-controller~=${controllerName}]`)
 
-    if (!parentControllerElement) {
+    const elementPrevisoulyHadStimulusReflexController = (element === parentControllerElement && controllerName === 'stimulus-reflex')
+
+    if (!parentControllerElement || elementPrevisoulyHadStimulusReflexController) {
       controllers.push(controllerName)
     }
   })

javascript/test/attributes.attributeValue.test.js

@@ -31,6 +31,13 @@ describe('attributeValue', () => {
     )
   })
 
+  it('returns expected attribute value for array with duplicate values', () => {
+    assert.equal(
+      attributeValue(['one', 'two', 'three', 'three', 'two', 'one']),
+      'one two three'
+    )
+  })
+
   it('returns expected attribute value for array with mixed and duplicate values', () => {
     assert.equal(
       attributeValue([

javascript/test/controllers.allReflexControllers.test.js

@@ -0,0 +1,103 @@
+import { html, fixture, assert, nextFrame } from '@open-wc/testing'
+import refute from './refute'
+
+import ExampleController from './dummy/example_controller'
+import RegularController from './dummy/regular_controller'
+
+import { initialize } from '../stimulus_reflex'
+
+import App from '../app'
+import { application } from './dummy/application'
+import { allReflexControllers } from '../controllers'
+
+import {
+  unloadAllControllers,
+  registeredControllers,
+  identifiers
+} from './test_helpers'
+
+describe('allReflexControllers', () => {
+  beforeEach(() => {
+    initialize(application)
+  })
+
+  afterEach(() => {
+    unloadAllControllers()
+  })
+
+  it('returns StimulusReflex-enabled controller from parent', async () => {
+    App.app.register('sr', ExampleController)
+
+    assert.deepEqual(registeredControllers(), ['stimulus-reflex', 'sr'])
+
+    const element = await fixture(html`
+      <div data-controller="sr">
+        <a></a>
+      </div>
+    `)
+
+    const a = element.querySelector('a')
+    assert.deepEqual(identifiers(allReflexControllers(a)), ['sr'])
+  })
+
+  it('doesnt return regular controller from parent', async () => {
+    App.app.register('regular', RegularController)
+
+    assert.deepEqual(registeredControllers(), ['stimulus-reflex', 'regular'])
+
+    const element = await fixture(html`
+      <div data-controller="regular">
+        <a></a>
+      </div>
+    `)
+
+    const a = element.querySelector('a')
+    assert.isEmpty(identifiers(allReflexControllers(a)))
+  })
+
+  it('should return all reflex controllers from parents', async () => {
+    App.app.register('sr-one', ExampleController)
+    App.app.register('sr-two', ExampleController)
+    App.app.register('regular-one', RegularController)
+    App.app.register('regular-two', RegularController)
+
+    const element = await fixture(html`
+      <div data-controller="sr-one">
+        <div data-controller="sr-two">
+          <div data-controller="regular-one">
+            <div data-controller="regular-two">
+              <a></a>
+            </div>
+          </div>
+        </div>
+      </div>
+    `)
+
+    const a = element.querySelector('a')
+
+    const controllers = allReflexControllers(a)
+
+    assert.deepEqual(identifiers(controllers), ['sr-two', 'sr-one'])
+  })
+
+  it('should return controllers with same name', async () => {
+    App.app.register('sr', ExampleController)
+
+    const outer = await fixture(html`
+      <div data-controller="sr" id="outer">
+        <div data-controller="sr" id="inner">
+          <a></a>
+        </div>
+      </div>
+    `)
+
+    const a = outer.querySelector('a')
+    const inner = outer.querySelector('#inner')
+    const controllers = allReflexControllers(a)
+
+    assert.deepEqual(identifiers(controllers), ['sr', 'sr'])
+
+    assert.deepEqual(controllers[0].element, inner)
+    assert.deepEqual(controllers[1].element, outer)
+  })
+})

javascript/test/controllers.extractReflexName.test.js

@@ -5,23 +5,25 @@ import { findControllerByReflexName } from '../controllers'
 
 describe('findControllerByReflexName', () => {
   it('returns undefined if empty controllers array is passed', () => {
-    assert.equal(
-      findControllerByReflexName('click->TestReflex#create', []),
-      undefined
+    assert.isUndefined(
+      findControllerByReflexName('click->TestReflex#create', [])
     )
+    assert.isUndefined(findControllerByReflexName('click->Test#create', []))
   })
 
-  it('returns first controller if no matching controller is found', () => {
+  it('returns no controller if no matching controller is found', () => {
     const controller = { identifier: 'test' }
     const controllers = [
       { identifier: 'first' },
       controller,
       { identifier: 'last' }
     ]
 
-    assert.equal(
-      findControllerByReflexName('click->NoReflex#create', controllers),
-      controllers[0]
+    assert.isUndefined(
+      findControllerByReflexName('click->NoReflex#create', controllers)
+    )
+    assert.isUndefined(
+      findControllerByReflexName('click->No#create', controllers)
     )
   })
 
@@ -37,6 +39,11 @@ describe('findControllerByReflexName', () => {
       findControllerByReflexName('click->TestReflex#create', controllers),
       controller
     )
+
+    assert.equal(
+      findControllerByReflexName('click->Test#create', controllers),
+      controller
+    )
   })
 
   it('returns matching namespaced controller', () => {
@@ -54,6 +61,14 @@ describe('findControllerByReflexName', () => {
       ),
       controller
     )
+
+    assert.equal(
+      findControllerByReflexName(
+        'click->Some::Deep::Module::Test#create',
+        controllers
+      ),
+      controller
+    )
   })
 
   it('returns dasherized controller', () => {
@@ -68,5 +83,10 @@ describe('findControllerByReflexName', () => {
       findControllerByReflexName('click->SomeThingReflex#create', controllers),
       controller
     )
+
+    assert.equal(
+      findControllerByReflexName('click->SomeThing#create', controllers),
+      controller
+    )
   })
 })

javascript/test/controllers.localReflexControllers.test.js

@@ -0,0 +1,78 @@
+import { html, fixture, assert, nextFrame, oneEvent } from '@open-wc/testing'
+
+import { application } from './dummy/application'
+import ExampleController from './dummy/example_controller'
+import RegularController from './dummy/regular_controller'
+
+import App from '../app'
+import { localReflexControllers } from '../controllers'
+import { initialize } from '../stimulus_reflex'
+
+import {
+  unloadAllControllers,
+  registeredControllers,
+  identifiers
+} from './test_helpers'
+
+describe('localReflexControllers', () => {
+  beforeEach(() => {
+    initialize(application)
+  })
+
+  afterEach(() => {
+    unloadAllControllers()
+  })
+
+  it('returns StimulusReflex-enabled controller', async () => {
+    App.app.register('sr', ExampleController)
+
+    assert.deepEqual(registeredControllers(), ['stimulus-reflex', 'sr'])
+
+    const element = await fixture(html`
+      <div data-controller="sr"></div>
+    `)
+
+    assert.deepEqual(identifiers(localReflexControllers(element)), ['sr'])
+  })
+
+  it('doesnt return regular controller', async () => {
+    App.app.register('sr', ExampleController)
+    App.app.register('regular', RegularController)
+
+    assert.deepEqual(registeredControllers(), [
+      'stimulus-reflex',
+      'sr',
+      'regular'
+    ])
+
+    const element = await fixture(html`
+      <div data-controller="sr regular"></div>
+    `)
+
+    assert.deepEqual(identifiers(localReflexControllers(element)), ['sr'])
+  })
+
+  it('returns all StimulusReflex-enabled controllers', async () => {
+    App.app.register('sr-one', ExampleController)
+    App.app.register('sr-two', ExampleController)
+    App.app.register('regular-one', RegularController)
+    App.app.register('regular-two', RegularController)
+
+    assert.deepEqual(registeredControllers(), [
+      'stimulus-reflex',
+      'sr-one',
+      'sr-two',
+      'regular-one',
+      'regular-two'
+    ])
+
+    const element = await fixture(html`
+      <div data-controller="regular-two sr-two sr-one regular-one"></div>
+    `)
+
+    assert.deepEqual(identifiers(localReflexControllers(element)), [
+      'sr-two',
+      'sr-one'
+    ])
+  })
+})

javascript/test/reflexes.setupDeclarativeReflexesForElement.test.js

@@ -9,17 +9,15 @@ import { initialize } from '../stimulus_reflex'
 import App from '../app'
 import { scanForReflexesOnElement } from '../scanner'
 
-function registeredControllers () {
-  return Array.from(App.app.router.modulesByIdentifier.keys())
-}
+import { unloadAllControllers, registeredControllers } from './test_helpers'
 
 describe('scanForReflexesOnElement', () => {
   beforeEach(() => {
     initialize(application)
   })
 
   afterEach(() => {
-    App.app.unload(registeredControllers())
+    unloadAllControllers()
   })
 
   it('should add the right action and controller attribute', async () => {
@@ -322,4 +320,44 @@ describe('scanForReflexesOnElement', () => {
     assert.equal(button.dataset.action, 'click->example#__perform')
     assert.equal(button.dataset.controller, undefined)
   })
+
+  it('should remove stimulus-reflex controller when other controller is a matching StimulusReflex-enabled controller', async () => {
+    App.app.register('example', ExampleController)
+
+    const controllerElement = await fixture(html`
+      <div
+        data-controller="example stimulus-reflex"
+        data-reflex="click->Example#else"
+      ></div>
+    `)
+
+    scanForReflexesOnElement(controllerElement)
+
+    assert.equal(controllerElement.dataset.controller, 'example')
+    assert.equal(controllerElement.dataset.reflex, 'click->Example#else')
+    assert.equal(controllerElement.dataset.action, 'click->example#__perform')
+  })
+
+  it('should not remove stimulus-reflex controller when other controller is a non-matching StimulusReflex-enabled controller', async () => {
+    App.app.register('example', ExampleController)
+
+    const controllerElement = await fixture(html`
+      <div
+        data-controller="example stimulus-reflex"
+        data-reflex="click->Something#else"
+      ></div>
+    `)
+
+    scanForReflexesOnElement(controllerElement)
+
+    assert.equal(
+      controllerElement.dataset.controller,
+      'example stimulus-reflex'
+    )
+    assert.equal(controllerElement.dataset.reflex, 'click->Something#else')
+    assert.equal(
+      controllerElement.dataset.action,
+      'click->stimulus-reflex#__perform'
+    )
+  })
 })

javascript/test/test_helpers.js

@@ -0,0 +1,13 @@
+import App from '../app'
+
+export function registeredControllers () {
+  return Array.from(App.app.router.modulesByIdentifier.keys())
+}
+
+export function unloadAllControllers () {
+  App.app.unload(registeredControllers())
+}
+
+export function identifiers (controllers) {
+  return controllers.map(controller => controller.identifier)
+}