Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Outlets not available in connect/disconnect #618

Closed
drjayvee opened this issue Dec 5, 2022 · 22 comments · Fixed by #648
Closed

Outlets not available in connect/disconnect #618

drjayvee opened this issue Dec 5, 2022 · 22 comments · Fixed by #648
Assignees

Comments

@drjayvee
Copy link
Contributor

drjayvee commented Dec 5, 2022

When I try to test Outlets using the example from the docs, I get the following error:

The provided outlet element is missing the outlet controller "result" for "search" 
<div class="result" data-controller="result">

I've prepared a JSFiddle to demonstrate. Here's the full code

<!DOCTYPE html>
<html lang="en">
<head>
  <title>Outlets tehst</title>
  <script type="module">
    import { Application, Controller } from 'https://unpkg.com/@hotwired/stimulus/dist/stimulus.js'

    const app = Application.start()
    app.debug = true;
    
    app.register('search', class extends Controller {
      static outlets = ['result']

      connect() {
        console.log(this.resultOutlets)
        this.element.insertAdjacentHTML('beforeend', `. Found ${this.resultOutlets.length} outlets`);
      }
    });

    app.register('result', class extends Controller {});
  </script>
</head>

<body>
  <div
    data-controller="search"
    data-search-result-outlet=".result"
  >Search</div>
  
  <div>
    <div class="result" data-controller="result">Result</div>
    <div class="result" data-controller="result">Result</div>
  </div>
</body>
</html>

I've done a little debugging and found that Router.getContextForElementAndIdentifier finds a module with no contexts (empty Array), which quickly leads to the error above.

I've tried versions 3.2.0 and 3.2.1. Neither works.

@NakajimaTakuya
Copy link
Contributor

NakajimaTakuya commented Dec 5, 2022

This seems to be a timing issue.
I tried delaying it a bit and got it right.
It may be a good idea to modify the sample code in the documentation.
https://codepen.io/nazomikan/pen/zYayvNJ?editors=1010

@drjayvee
Copy link
Contributor Author

drjayvee commented Dec 5, 2022

Thanks for pointing out the workaround.

I'd argue that this is a bug in the implementation. The values/targets/classes APIs don't require waits, so this is hugely surprising.

The error message also seems totally buggy since its message complains about the controller missing, when the very message points out that the data-controller attribute is present and correct.

At the very least, the timing issue should be documented.

@dhh
Copy link
Member

dhh commented Dec 5, 2022

cc @marcoroth

@marcoroth
Copy link
Member

I will check out what might be wrong here, thanks for opening this issue @drjayvee!

@marcoroth marcoroth self-assigned this Dec 22, 2022
@edariedl
Copy link

edariedl commented Jan 7, 2023

Same thing is happening to us in the connect we have also encountered similar issue in disconnect when using Turbo Drive. Outlet can be disconnected before the controller which causes same error as above.

has[identifier]Outlet also returns true even though outlet isn't there which makes it harder to check for the situation. [identifier]Outlets.length can be used as a workaround even though it shows warning with the same text as an error in the javascript console (at least in safari).

setTimeout workaround unfortunately didn't work in our case when lazily loading several controllers via importmaps from individual javascript files. It can be hard to set delay properly if we don't know when the outlet controller will be loaded.

@drjayvee drjayvee changed the title Outlets example from docs doesn't work Outlets not available in connect/disconnect Jan 9, 2023
@jorismak
Copy link

I guess I'm running into the same or something similar?

I have a huge form with a stimulus-controller. That controller has an outlet for another controller somewhere in the middle of the form.

When the page is loaded, I get the error 'missing data-controller attribute on outlet element'.

When I look at the debugging info in the console, the outlet controller hasn't one it's connect() yet. So, in a way, the controller isn't there yet. So I understand the message. But it shows up as a big red blob.

My code uses the foobarOutletConnected() callback, and this works fine!
So, I'm trying to code it as an optional outlet, yet I still get the error that the controller attribute isn't there.

(Note, there is nothing really dynamic about how the outlet-controller gets inserted into the DOM. It's the same static HTML, it's just a big form, and by the time my form-controller is doing stuff, the DOM isn't loaded completely and the outlet-controller isn't there yet apparently.).

@apphancer
Copy link

I am encountering the same problem. Adding in a delay such as the sample code provided by @NakajimaTakuya does not appear to resolve it for me.

@jorismak
Copy link

I am encountering the same problem. Adding in a delay such as the sample code provided by @NakajimaTakuya does not appear to resolve it for me.

A delay would only be an ugly hack. Basically, you are fighting a race condition. Instead of delaying, do it the proper way.

Use the xxxxOutletConnected() callback. Then you know it's there.

There is still the weird case that 'hasxxxxOutlet' can return true, while 'xxxOutlets.length' will still return 0.

So, the code that wants to use the outlet (for example the foobar outlet), wrap it in something like

if (this.foobarOutlets.length > 0) {
   // access this.foobarOutlet or this.foobarOutlets
}

and I add another method for when the outlet comes in later when the DOM is fully parsed.

foobarOutletConnected() {
    this.updateStuffWithOutlet();
}

So, to prevent the error in the console, do not use this.hasFoobarOutlet() but use this.foobarOutlets.length.
And make sure you use the foobarOutletConnected() callback in case the outlet will 'arrive later', which it almost always will.

@EternalPatience
Copy link

EternalPatience commented Jan 25, 2023

We are experiencing the same issue. There are two controllers, each with outlets linking to the other - the modal and overlay controllers. While there is only one overlay, there are many modals. In the modal controller, there is a method called open() which can be executed upon connection:

open() {
  this.overlayOutlet.show();
  this.overlayOutlet.openModalOutlet?.hide();
  this.show();
}

The overlayOutlet.openModalOutlet?.hide() command closes all other modals. However, we are encountering a problem when not all modal outlets are connected to the Overlay controller, as not all Modal Controllers are.
It is surprising to me why the method of the ModalController is executed before all the modal controllers are connected.

@marcoroth
Copy link
Member

marcoroth commented Jan 27, 2023

The problem is the order how Stimulus connects controllers. So this issue is dependent how the elements are found in the DOM and in which order the MutationObserver processes them.

So if you try to access the outlets in the connect() action of the "host controller" and the dependent outlets haven't connected yet you will get the The provided outlet element is missing the outlet controller "result" for "search" error.

This won't work:

<div data-controller="search" data-search-result-outlet=".result">Search</div>
<div class="result" data-controller="result">Result</div>
<div class="result" data-controller="result">Result</div>

However, this works as intended.

<div class="result" data-controller="result">Result</div>
<div class="result" data-controller="result">Result</div>
<div data-controller="search" data-search-result-outlet=".result">Search</div>

But in both scenarios you can access the outlets after the dependent outlet controllers have connected. Also the outlet callbacks work as expected.

There are few options how we can solve that you can access outlets in any case, independent of DOM order appearance:

    1. We try to scan all element instances on the page which have a data-controller attribute attached, build a "dependency tree", sort the elements to be connected accordingly and then connect them in that order. This works if we disallow circular outlet dependencies.
    1. When the outlets are accessed and we can't find them (this is where the error message appears now) we can try to find the element with the provided selector and see if it would have a matching data-controller attribute. If it does, we could connect that controller on-the-fly and enforce it that way. However, there is the downside that it might try to connect that controller again at a later point, because it was scheduled to be connected. But maybe we can also enhance the "connect logic" in that step to make sure there isn't a connected controller already and just disregard the second connect attempt.
    1. We don't allow outlets to be accessed in the connect(), but this feels like a weird limitation.
    1. We make the accessor for this.[name]Outlet and this.[name]Outlets async and make them return a promise which resolves as soon as the dependent controllers are connected. But this would probably lead to a breaking change.

I'll investigate which option is the best solution here. Personally my vote would go for option ii. But happy to hear what others think.

@jorismak
Copy link

jorismak commented Jan 27, 2023 via email

@drjayvee
Copy link
Contributor Author

drjayvee commented Jan 30, 2023

But happy to hear what others think.

I'd very much prefer any solution which is transparent to users and consistent with other features.

Therefore, I'd rate solutions III and IV as "definitely meh" ®.

Solution I isn't all that difficult to implement. If a circular dependency is detected, the code can just fall back to default ordering and 🤞.

II seems like the most robust - if you can implement it properly. Not judging your skill here, but it seems complicated and could regress when core Stimulus DOM / mutation observers change slightly. Hopefully this can be thoroughly tested. I'll leave that up to you (since I can't make sense of the code at all 😄.)

Joris makes a good point re: lazy loading. Ideally, the solution should support that transparently as well!

[Edit: II is the most robust, not III]

@marcoroth
Copy link
Member

III seems like the most robust

@drjayvee I hope you meant to say ii here 🙈 But I would agree with you in general.

In any case, I started to explore ii in #648, which looks promising so far.

@spthiel
Copy link

spthiel commented Mar 28, 2023

To note on this: this calls a warning The provided outlet element is missing the outlet controller "<outlet controller>" for "<controller>" regardless if it's called in connect or not. Which should be resolved with implementation i and ii but not with iii or iv

@promisedlandt
Copy link

The error message also seems totally buggy since its message complains about the controller missing, when the very message points out that the data-controller attribute is present and correct.

Stumbled over this today. In my case, it was with a multi-entry data-controller attribute (see codepen. The error message first made me think that controllers used as outlets need to be declared as a single-entry data-controller attribute.
It feels a lot like the message doesn't describe the error that occurred, but the most likely cause for the error that occurred.

Since fixing the root cause seems non-trivial, maybe changing the error message could be a good quickfix?

@marcoroth
Copy link
Member

@promisedlandt I'm open to update the error message if you have an idea on how to improve it.

Luckily, the root cause is fixed and already implemented with #648, but I haven't had the time to finish it up yet. I'm looking to finish that up soon!

@promisedlandt
Copy link

@promisedlandt I'm open to update the error message if you have an idea on how to improve it.

I think the error message would be slightly improved by:

"Unable to find a controller when searching for the "result" outlet of the "search" controller. Ensure CSS selector "#result" is correct and target element has a data-controller attribute that includes "result", e.g. "data-controller=result"."

(Not sure how easy including the CSS selector woud be, it's not particularily important)

Speaking to my original point: if you're planning a release soon-ish without #648 I would suggest adding another senctence to the error message with an explicit reference to this issue , i.e.
"If the target is correctly declared as the "result" controller, it might not be connected yet. See #618 for a workaround."

@lingceng
Copy link

lingceng commented Apr 26, 2023

By wrapping codes in a setTimeout avoid this warning for me:

  messageOutletConnected(outlet, element) {
    // Run in next tick to wait message outlet controller ready
    setTimeout(() => {
      let lastAnswer = this.messageOutlets[this.messageOutlets.length - 1]
      this.isWaiting = lastAnswer.statusValue == "waiting"
      this.handleStatus()
    }, 0)
  }

@rgarner
Copy link

rgarner commented Jun 27, 2023

I'm getting this error also, and have gone to the ...OutletConnected() method to get moving. If it's the case that ...OutletConnected() is required, then the example at https://stimulus.hotwired.dev/reference/outlets#definitions should be updated so that it doesn't suggest that iterating outlets in a connect() is canon.

Happy to PR those docs if someone can point me at them.

@marcoroth
Copy link
Member

marcoroth commented Jun 27, 2023

Hey @rgarner, #648 fixed the underlying issue. If you want to give it a shot feel free to pull in the dev-build to double check if it also solves your use-case.

yarn add @hotwired/stimulus@https://github.com/hotwired/dev-builds/archive/@hotwired/stimulus/7bf453c.tar.gz

@rgarner
Copy link

rgarner commented Jun 28, 2023

Thanks @marcoroth - even better!

@drjayvee
Copy link
Contributor Author

drjayvee commented Aug 3, 2023

@dhh could you please create a new release which includes the fix for this issue? Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

13 participants