stimulus-loading Race Condition

[tleish]

In trying to use importmap with stimulus `stimulus-loading' and I've run into a challenge with race conditions.

For example, say I have the following code.

// send_controller.js
export default class extends Controller {
  connect() {
    this.dispatch('connected')
  }
}

// receive_controller.js
export default class extends Controller {
  catch() {
    console.log('catch');
  }
}

<div data-controller="receive send"
     data-action="send:connected->receive#catch"></div>

1. send controller dispatches a connected event on connect()
2. send controller event runs action receive#catch

In the case of importmap, sometimes the send controller loads and executes before receive controller loaded. The event is dispatched but nothing happens because the receive controller is not loaded.

I've explored other options:

Option 1: turbo:load@window->receive#catch

This works for visits, but not not if the content is loaded via turbo-frame or a FORM turbo-stream response with lazyLoadControllersFrom

Option 2: turbo:load@window->receive#catch turbo:frame-load@window->receive#catch

This works for visits and turbo-frame, but still not for FORM turbo-stream response with lazyLoadControllersFrom.

I thought perhaps I could tap into the solution outlined in #57, but the PR is closed.

Is there a solution I'm not aware of that works with visit, turbo-frame and turbo-stream scenarios? Perhaps an event which indicates that all the stimulus controllers have been loaded and ready to execute? Should stimulus hold off on executing javascript until controllers finish loading?

[dhh]

Hmm, not an easy problem to solve, because lazy loading is loading things dynamically. So some controllers might have been loaded already, and good to go. Others only load when new HTML appears that have them in there.

Seems like what we need is essentially turbo:finish or something like that which captures all forms of turbo loading. Do explore that in Turbo 👍

[tleish]

Do explore that in Turbo

The dynamic loading of the stimulus controllers occurs in the stimulus-loading.js
library in this codebase. Would it then make more sense to include the logic in the library which dynamically loads the stimulus controllers?

Basing it off this closed PR created by @seanpdoyle, the following modifications dispatches an event stimulus:ready which solves the issue for us:

function loadController(name, under, application) {
  if (!(name in registeredControllers)) {
+   registeredControllers[name] = false
    import(controllerFilename(name, under))
      .then(module => registerController(name, module, application))
-     .catch(error => console.error(`Failed to autoload controller: ${name}`, error))
+     .catch(error => importError(error, name))
+     .finally(()  => dispatchWhenReady())
  }
}

function registerController(name, module, application) {
- if (!(name in registeredControllers)) {
+ if (!registeredControllers[name]) {
    application.register(name, module.default)
    registeredControllers[name] = true
  }
}

+ function importError(error, name) {
+   delete registeredControllers[name]
+   console.error(`Failed to autoload controller: ${name}`, error)
+ }

+ function dispatchWhenReady() {
+   if(Object.values(registeredControllers).every(Boolean)) {
+     const controllers = Object.keys(registeredControllers);
+     const event = new CustomEvent('stimulus:ready', { detail: { controllers }, bubbles: true });
+     document.dispatchEvent(event);
+   }
+ }

[dhh]

Actually, yes, I do like that option too. I was thinking of turbo:finish, which I also think we should explore. But I'd take the stimulus:ready event too. That's a good idea.

[drewlustro]

Hello @dhh @tleish – Is there any continued interest on this one?

I also would be quite interested in a stimulus:ready event (per controller), and perhaps a turbo:finish (once all loading complete). I checked out stimulus-loading.js and I didn't see an event emitted.

On our application, we've moved to lazyLoadControllersFrom for performance reasons, but are noticing a 300–700ms delay from seeing HTML to the page actually being ready for interaction (controllers loaded).

Of course, it could be because we're still loading much unnecessary JS prior to MutationObserver kicking in effect, so maybe it's a "main thread blocked for too long" issue.

In any case, stimulus emitting such an event on lazy load would help validate any hypothesis/diagnostic.

[dhh]

I'm actually starting to think that maybe we shouldn't lazy load at all. That it just makes things too complicated. The majority of apps should just preload all controllers. And the ones who can't do that can just have different includes per page. I think scanning the HTML to lazy load is too clever, too slow, and too error prone.

We already changed the default to eager loading.

[tleish]

it just makes things too complicated

@dhh - Curious what problems you are running into. We occasionally need to handle order dependencies of cross-controller communication using lazy loading, but it hasn't been a major issue.

We see quite a difference in performance initial page load when testing using speed test using lighthouse, we see numbers like:

• eager load: 68 lighthouse performance score
• lazy load: 95 lighthouse performance score

[dhh]

Problem is attachment timing. When there are things in the DOM that need to change from a controller. I seem to remember us getting visual jitter from it.

Haven't seen anything like that on lighthouse score differences. Is that a CDN issue maybe?

[tleish]

Numbers posted above are just from running localhots. Only difference between the to runs is the use of @hotwired/stimulus-loading#eagerLoadControllersFrom vs @hotwired/stimulus-loading#lazyLoadControllersFrom

[dhh]

Gotcha. If you actually need those controllers loaded, and they operate on the DOM, I think that difference is artificial.

[tleish]

We have lots of stimulus controllers and js libraries. We need a portion of the controllers for a given page, but not all of them.

For example, I see hey.com has over 230 js modules, with over 150 of them as controllers. Managing which controller to load on which page seems like a pain. Loading all 230 files on every page load also seems overkill, even with http2. Dynamically lazy loading the modules as they are used in the DOM makes it so much easier from a development perspective, cleaner and faster from a user/browser loading experience. Especially when some of those controllers might include large js modules.

I assume this is why hey.com also uses lazyloading:

/* app.hey.com/assets/controllers/index.js */
import { application } from "controllers/application"
import { lazyLoadControllersFrom } from "@hotwired/stimulus-loading"

lazyLoadControllersFrom("controllers", application)

It's a slick strategy.

[dhh]

The HEY usage is actually just legacy from the old defaults. I don't think we'd do it like this today. HTTP/2 really does solve the majority of it. For Campfire we switched to eager loading. But would be happy to see this covered in more detail. The trade offs. Both in terms of managing timing, DOM mutation, and bandwidth.

On March 12, 2024, "github.com" ***@***.***> wrote: We have lots of stimulus controllers and js libraries. We need a portion of the controllers for a given page, but not all of them. For example, I see hey.com has over 230 js modules, with over 150 of them as controllers. Managing which controller to load on which page seems like a pain. Loading all 230 files on every page load also seems overkill, even with http2. Dynamically lazy loading the modules as they are used in the DOM makes it so much easier from a development perspective, cleaner and faster from a user/browser loading experience. Especially when some of those controllers might include large js modules. I assume this is why hey.com also uses lazyloading: /* app.hey.com/assets/controllers/index.js */ import { application } from "controllers/application" import { lazyLoadControllersFrom } from ***@***.***/stimulus-loading" lazyLoadControllersFrom("controllers", application) It's a slick strategy.

— Reply to this email directly, view it on GitHub <#93 (comment)- 1992772085>, or unsubscribe <https://github.com/notifications/unsubscribe- auth/AAAAVNJB466FXW5JGYKGZ6DYX6KC7AVCNFSM5NDY5HL2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOJZGI3TOMRQHA2Q>. You are receiving this because you were mentioned.Message ID: ***@***.***>