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

stimulus-reflex:ready event should only be emitted once per page load #608

Closed
leastbad opened this issue Sep 6, 2022 · 4 comments · Fixed by #625
Closed

stimulus-reflex:ready event should only be emitted once per page load #608

leastbad opened this issue Sep 6, 2022 · 4 comments · Fixed by #625
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed javascript Pull requests that update Javascript code proposal
Milestone

Comments

@leastbad
Copy link
Contributor

leastbad commented Sep 6, 2022

Feature Request

Currently, the stimulus-reflex:ready event is emitted every time setupDeclarativeReflexes() is called. There are two concerns:

  1. Someone might reasonably expect this to only be called once, leading to multiple initializations of app-level functions.
  2. We might eventually remove the readystatechange handler in favour of just letting each Stimulus controller register itself... which means that calling the event when importmaps are in play could cause the event to fire before the controllers are actually ready.

Is your feature request related to a problem?

Ties into the solution provided by #606.

Describe the solution you'd like

The simple approach is to use a variable to track whether the event has been emitted, and then return if it's about to be emitted again. The complex approach - which I don't have a solution for due to my relative inexperience with importmaps - is to only emit the event when all controllers have registered. We don't currently have a mechanism to track this.

@leastbad leastbad added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers proposal javascript Pull requests that update Javascript code labels Sep 6, 2022
@leastbad leastbad added this to the 3.5 milestone Sep 6, 2022
@marcoroth
Copy link
Member

I don't think this should be a big deal, since we just exported that function a few commits ago in #602.

So it's never actually called twice unless you are explicitly importing the setupDeclarativeReflexes() function and calling it yourself in your application.

One solution might be to just move the emitEvent('stimulus-reflex:ready') to the eventlistener in javascript/stimulus_reflex.js:

document.addEventListener('readystatechange', () => {
  if (document.readyState === 'complete') {
    setupDeclarativeReflexes()
    emitEvent('stimulus-reflex:ready')
  }
})

@leastbad
Copy link
Contributor Author

leastbad commented Sep 6, 2022

So it's never actually called twice unless you are explicitly importing the setupDeclarativeReflexes() function and calling it yourself in your application.

I don't believe that's correct. We call it every time the mutation observer picks up new elements.

const observer = new MutationObserver(setupDeclarativeReflexes)

@marcoroth
Copy link
Member

Oh, right. But that would mean that we were also emitting the event multiple times for a long time.

@leastbad
Copy link
Contributor Author

leastbad commented Sep 6, 2022

Yes, stimulus-reflex:ready is a relatively recent addition. It's possible "very few" are using it, yet.

marcoroth added a commit that referenced this issue Jan 18, 2023
# Type of PR

Enhancement

## Description

Previously the `stimulus-reflex:ready` was invoked every time the
`scanForReflexes()` function was called. With the recent changes of
being able to load Stimulus controllers async we can't guarantee that
the `stimulus-reflex:ready` was always triggered at the right moment in
time and it wasn't guaranteed that all controllers were actually loaded
and registered.

Since we can't guarantee that the "whole document and all controllers
are loaded and ready", we instead emit an event on the actual element
that we know that it is ready and was setup for sure.

Resolves #608 

## Why should this be added

Consistently and better support for asynchronous loading of Stimulus
controllers.

## Checklist

- [x] My code follows the style guidelines of this project
- [x] Checks (StandardRB & Prettier-Standard) are passing
- [x] This is not a documentation update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed javascript Pull requests that update Javascript code proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants