disconnect() not called if outlet is accessed in connect() #763

sebastianludwig · 2024-04-21T17:07:21Z

I noticed that accessing an outlet in connect() causes the reference count of that controller to be 2 instead of 1 after start has finished. That means all actions which under normal circumstances cause the controller to be disconnected (like removing the data-controller attribute) don't cause the disconnect anymore.

The following code snippet illustrates the problem

<!doctype html>
<html>
<head>
  <meta charset="utf-8">
  <script type="module">
    import { Application, Controller } from "https://unpkg.com/@hotwired/stimulus/dist/stimulus.js"
    window.Stimulus = Application.start()

    Stimulus.register("child", class extends Controller {
      connect() {
        console.log("Child connected")
      }
      disconnect() {
        console.log("Child disconnected")
        alert("works as expected")
      }
    })

    Stimulus.register("parent", class extends Controller {
      static outlets = [ "child" ]
  
      connect() {
        console.log("Parent connected")
        console.log(this.childOutlet) // <- works without this line 💥
      }
      disconnect() {
        console.log("Parent disconnected")
      }
      
      listChildren() {
        console.log(this.childOutlet)
      }
    });

    document.getElementById("remove").addEventListener("click", function() {
      console.log("Removing data-controller attribute");
      document.getElementById("child").setAttribute("data-controller", "");
    });
  </script>
</head>
<body>
  <div id="parent" data-controller="parent" data-parent-child-outlet="#child">
    <button data-action="click->parent#listChildren">List children</button>
  </div>
  <div id="child" data-controller="child"></div>
  <button id="remove">Remove data-controller=child attribute</button>
</body>
</html>

Setting a breakpoint in ScopeObserver.elementMatchedValue() and reloading the page you can see that it is hit twice for child. Clicking the remove button does not trigger disconnect() to be called. Everything works as expected with the marked line commented out.

The text was updated successfully, but these errors were encountered:

ildarkayumov · 2024-04-28T09:36:28Z

Hi.

Looks like the problem occurs because of Outlet#get. So when you load outlet inside of parent controller there is no associated controller for child outlet element and stimulus tries to connect controller with element before show warning message The provided outlet element is missing an outlet controller...

function getControllerAndEnsureConnectedScope(controller: Controller, element: Element, outletName: string) {
  let outletController = getOutletController(controller, element, outletName)
  if (outletController) return outletController

  controller.application.router.proposeToConnectScopeForElementAndIdentifier(element, outletName)
  ...
}

https://github.com/hotwired/stimulus/blob/main/src/core/outlet_properties.ts#L21

sebastianludwig · 2024-05-04T19:44:23Z

I'm not sure that's the right track. At least I'm not seeing the warning in the console 🤔

Edit: It indeed seems to be related. In a similar situation I do get the warning. But I haven't investigated yet what the difference to the example in the issue is and why I'm not seeing the warning there.

ildarkayumov · 2024-05-06T06:42:25Z

please check this

function getControllerAndEnsureConnectedScope(controller: Controller, element: Element, outletName: string) {
  let outletController = getOutletController(controller, element, outletName)
  if (outletController) return outletController

  controller.application.router.proposeToConnectScopeForElementAndIdentifier(element, outletName)

  outletController = getOutletController(controller, element, outletName)
  if (outletController) return outletController
}

function propertiesForOutletDefinition(name: string) {
  const camelizedName = namespaceCamelize(name)

  return {
    [`${camelizedName}Outlet`]: {
      get(this: Controller) {
        const outletElement = this.outlets.find(name)
        const selector = this.outlets.getSelectorForOutletName(name)

        if (outletElement) {
          const outletController = getControllerAndEnsureConnectedScope(this, outletElement, name)

          if (outletController) return outletController

          throw new Error(
            `The provided outlet element is missing an outlet controller "${name}" instance for host controller "${this.identifier}"`
          )
        }
        ...
}

inside singular outlet controller getter getControllerAndEnsureConnectedScope is called and if it returns null then throws and error.
inside getControllerAndEnsureConnectedScope router method router.proposeToConnectScopeForElementAndIdentifier -> elementMatchedValue is called. So this is first time
plus 1 more time elementMatchedValue is called during Stimulus loading

by my opinion calling outlet inside parent_controller#connect does not seem like correct for me because controller loading is not finished yet
I guess the easiest "hacky" fix is to wrap childOutlet with setTimeout.

sebastianludwig · 2024-05-17T22:38:49Z

Mh, I can't reproduce the case where I saw the The provided outlet element is missing an outlet controller error. So I'm back to

I'm not seeing the "is missing an outlet controller" error.

@ildarkayumov do you see that error when running the provided example? Or can you provide an example which causes the error to be thrown?

I did find out one more detail: The example also works as expected if <div id="child"> is placed before the parent

<!-- pacing child here also works 💥 -->
<div id="child" data-controller="child"></div> 

<div id="parent" data-controller="parent" data-parent-child-outlet="#child">
  <button data-action="click->parent#listChildren">List children</button>
</div>

Regarding the question if accessing outlets in connect() should work or not:

ildarkayumov wrote:
by my opinion calling outlet inside parent_controller#connect does not seem like correct for me because controller loading is not finished yet

If we follow that, then it should be documented, should throw an error and not work 50:50 depending on the structure of the HTML.

However I'd argue that it should work. Accessing child controller properties to set an initial state (either in the parent controller or in the child controller) seems like a legitimate use case to me. The current documentation for outlets does exactly that: https://stimulus.hotwired.dev/reference/outlets#definitions

// chat_controller.js

export default class extends Controller {
  static outlets = [ "user-status" ]

  connect () {
    this.userStatusOutlets.forEach(status => ...)
  }
}

@marcoroth maybe you want to chime in? 😇

sebastianludwig · 2024-05-18T16:20:55Z

Well, I am able to reproduce the outlet element is missing an outlet controller error in one of my projects, but it depends on an import { foo } from "helper" being present in just one controller.js file or both. I was not able to distill this down into a small showcase. It looks like the dependency graph/load order plays a role..? 🤔

ildarkayumov · 2024-05-27T07:29:53Z

@sebastianludwig yes, you are right - there is a bug.
I debug it more and created a PR with small changes to prevent double elementMatchedValue

ildarkayumov linked a pull request May 25, 2024 that will close this issue

Do not ScopeObserver#scopeReferenceCounts when outlet is called before controller is connected #771

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disconnect() not called if outlet is accessed in connect() #763

disconnect() not called if outlet is accessed in connect() #763

sebastianludwig commented Apr 21, 2024

ildarkayumov commented Apr 28, 2024

sebastianludwig commented May 4, 2024 •

edited

ildarkayumov commented May 6, 2024 •

edited

sebastianludwig commented May 17, 2024 •

edited

sebastianludwig commented May 18, 2024

ildarkayumov commented May 27, 2024

disconnect() not called if outlet is accessed in connect() #763

disconnect() not called if outlet is accessed in connect() #763

Comments

sebastianludwig commented Apr 21, 2024

ildarkayumov commented Apr 28, 2024

sebastianludwig commented May 4, 2024 • edited

ildarkayumov commented May 6, 2024 • edited

sebastianludwig commented May 17, 2024 • edited

sebastianludwig commented May 18, 2024

ildarkayumov commented May 27, 2024

sebastianludwig commented May 4, 2024 •

edited

ildarkayumov commented May 6, 2024 •

edited

sebastianludwig commented May 17, 2024 •

edited