Fix for race condition in readystate detection

[ahollandECS]

This is an attempt to address the situation where the DOMContentLoaded event has already fired, but readyState has not yet reached "complete" (for example it can be "interactive"). In this case initialization will never happen because DOMContentLoaded will never fire again. If we're looking for a "complete" ready state, the DOMContentLoaded event is irrelevant, we should be looking for ready state changes.

[alexpetros]

Do you have any evidence that there is a race condition here? I don't think it's possible for DOMContentLoaded to have fired prior to executing the line of code that sets the event listener:

DOMContentLoaded will only run after all the deferred scripts have been executed, so as long as we listen for it on the initial execution of htmx, we should be safe. For good measure, I ORed it with getDocument().readystate === 'complete', in case someone is doing something insane like hotswapping the htmx script tag into the DOM based on some user action

It's certainly possible I missed something. More discussion here: #1688

[ahollandECS]

I have a page with this issue, it is absolutely possible. It is a fairly large page using the script.js async loader, so I don't have a portable example ready. But I have confirmed htmx is loading after DOMContentLoaded but before the page is "complete".

[alexpetros]

What is the script.js asnyc loader? That sounds like the issue. Can you provide a minimal code sample?

[ahollandECS]

The script loader (https://github.com/ded/script.js) almost definitely is the issue, but that's kind of beside the point. There are a lot of weird ways to get javascript onto a page and htmx should work with all of them.

I will try and get a minimal example, but again, there is a real issue and this fixes it.

[Telroshan]

The issue

Looking at readyState documentation

• interactive
  [...] sub-resources such as scripts, images, stylesheets and frames are still loading.
  The state indicates that the DOMContentLoaded event is about to fire.

• complete
  [...] all sub-resources have finished loading.
  The state indicates that the load event is about to fire.

So theoretically, there is a time frame between the moment the DOMContentLoaded event has been fired, and the moment that the document reaches the complete state. This time frame expands as you add heavy images or other heavy assets for ex and increase the amount of stuff to download before your page is complete

Looking at htmx' code:

htmx/src/htmx.js

Lines 3727 to 3730 in 953ce54

	var isReady = false
	getDocument().addEventListener('DOMContentLoaded', function() {
	isReady = true
	})

htmx/src/htmx.js

Line 3742 in 953ce54

if (isReady || getDocument().readyState === 'complete') {

• isReady will be set to true on DOMContentLoaded, but in this case where htmx is async loaded and could be downloaded after DOMContentLoaded has already been fired, isReady would remain forever false
• htmx could be downloaded during the time frame mentioned above, i.e. before the document reaches the complete state and while it's still in the interactive state

That means, in this specific scenario, because of async loading, htmx will bind to DOMContentLoaded (as isReady is false and the document's state is not complete), leading htmx to never initialize because DOMContentLoaded has already been triggered at this point.

I agree with @alexpetros that you would normally not want to load htmx async, as the "interactive" state would not be interactive after all if htmx has not loaded yet (which means all your hx-get on buttons and other stuff are still not working, and those define the interactions on your page), though I'm not aware of your situation so I don't know if async is a requirement in this case (if not, consider using defer instead!)

As for the PR

This PR works I think because the readystatechange event is fired both for the DOMContentLoaded and load events. So if htmx comes in the time frame mentioned above, then np the listener will be triggered anyway when load fires.
The problem I see with this change @ahollandECS

• If you have some massive image or video to load, it means htmx won't initialize before everything loads (load event), when we could maybe find another way to tell "well DOMContentLoaded already happened, initialize right away"?

I was going to suggest replacing getDocument().readyState === 'complete' by a getDocument().readyState !== 'loading' maybe?

• If the state is loading, it means DOMContentLoaded has not been fired yet, thus we can bind a listener to it without issue
• If the state is interactive or complete, then DOMContentLoaded has already been fired and we might just run the callback right away

Though as per the HTML spec

3. Update the current document readiness to "interactive".

6. [...] Fire an event named DOMContentLoaded

So technically, there is a little time frame in between the moment the interactive state is set, and the DOMContentLoaded event fires, so with my suggestion here, htmx could initialize right before DOMContentLoaded fires. Would that be an issue though? I honestly don't know, I'm not familiar with async scripts setups (by instinct I would say that it doesn't really matter since as the script was async anyway, you wouldn't care about execution order?)

Let me know what you think about this

[ahollandECS]

The issue with changing the ready state check to !== "loading" is that the exact opposite change happened a couple of months ago for affecting module imports: https://github.com/bigskysoftware/htmx/pull/1672/files and I don't think this change will regress that.

If it helps, here is a scratchpad with an example where htmx will consistently fail to initialize due to the ordering of DOMContentLoaded and readyState events: https://codepen.io/Andrew-Holland-the-scripter/pen/OJdpzrz

[ahollandECS]

If we want to keep the same performance in the usual use case, we could listen for both DOMContentLoaded and readystatechange => complete and use whichever happens first. I don't mind updating the PR with that if it would be accepted.

[Telroshan]

Thanks for the codepen!

the exact opposite change happened a couple of months ago for affecting module imports

You're right, I had completely forgotten about this, what a nightmare those initialization events are ; nevermind what I suggested indeed!

we could listen for both DOMContentLoaded and readystatechange => complete and use whichever happens first.

I agree, though we should probably listen for readystatechange only if the current document.readyState is already set to interactive, right? Otherwise we could bind a listener during the loading phase, causing that listener to be called when the state goes to interactive so before DOMContentLoaded is fired, if I'm not mistaken?

Edit: that's precisely what you suggested, wow am I bad at reading

[alexpetros]

The script loader (https://github.com/ded/script.js) almost definitely is the issue, but that's kind of beside the point. There are a lot of weird ways to get javascript onto a page and htmx should work with all of them.

It's beside the point only if we can accommodate it without regressions to the other ways that we currently support of getting JavaScript onto a page.

Generally speaking, the "correct" way to use htmx is use a script tag, and as @Telroshan points out, we also allow using the defer attribute on your script tag. We also have a leaning tower of weird old module support (no offense to AMD, CommonJS, and so on) and while I don't have an objection to accommodating script.js, it's a little tough for me to confidently verify that the change you're proposing wouldn't affect any of the more common module loaders that we already support. If I can't do that, I'm perfectly comfortable recommending (although it's not my final decision) that htmx says "hey, we support all these module loading methods, ymmv beyond that".

I am particularly concerned about performance regressions, and also regressing on #1688. Because of the close-but-not-exactly relationship between the document readystate and the events firing, which @Telroshan laid out in detail above, this cycle is quite tricky to get correct.

If we want to keep the same performance in the usual use case, we could listen for both DOMContentLoaded and readystatechange => complete and use whichever happens first. I don't mind updating the PR with that if it would be accepted.

I obviously can't promise that it would be accepted, but I would like to see it if you're willing.

[ahollandECS]

As discussed, I pushed changes to check both readystate and DOMContentLoaded. Whichever happens first will consume the pending initialization events.

[1cg]

i hate javascript so much

[1cg]

i didn't really grok the original change alex made at the time, but reading through the docs a bit more it seems to me we might be able to do something like this:

        function ready(fn) {
            if (document.state === "loading") {
                // trigger on the next readystatechange event which will be at least 'interactive'
                getDocument().addEventListener('readystatechange', fn, {once:true});
            } else {
                fn();
            }
        }

where we use the readystate events rather than DomContentLoaded.

i'm a total newb to all this, but my understanding is that the 'interactive' state is where we can start doing all our DOM processing, event handlers, etc. and this would narrow things down to just that window beetween loading and interactive.

please let me know what you think and also consider that javascript must be destroyed

[1cg]

Verified on Chrome, Safari and Firefox OSX that readystatechange only fires when the document transitions to interactive and complete

[1cg]

updated proposed ready() function using the new events area man was previously unaware even existed:

        function ready(fn) {
            if (document.state === "loading") {
                // trigger on the next readystatechange event which will be at least 'interactive'
                getDocument().addEventListener('readystatechange', fn, {once:true});
            } else {
                fn();
            }
        }

[Telroshan]

Yeah that's what I thought at first too @1cg, but as @ahollandECS correctly pointed out, that !== "loading" check is precisely what was changed in the past with @alexpetros' PR, where the problem was :

If HTMX is imported in a module the readyState is "interactive" so the extension processing happens too late.

So that line

 if (document.state === "loading") 

would cause that issue again as htmx would initialize right away, i.e. "too soon"

From what I understand with this past PR, even though the state was already "interactive", the DOMContentLoaded event itself had not been fired at this point, hence the event listener that still works

[1cg]

OK, @ahollandECS I'm sorry it took me a while to get up to speed, but @Telroshan walked me through everything. What do you think about the following code:

        function ready(functionToCall) {
            // if the page is complete, go ahead and call it
            if (getDocument().readyState === 'complete') {
                functionToCall();
            } else {
                // otherwise listen for DOMContentLoaded *and* readystatechange to "complete"
                // and call the function once in whichever event comes first
                getDocument().addEventListener('DOMContentLoaded', function() {
                  if(functionToCall){
                    functionToCall()
                    functionToCall = null;
                  }
                });
                getDocument().addEventListener('readystatechange',
                  if(functionToCall && getDocument().readyState === 'complete'){
                    functionToCall()
                    functionToCall = null;
                  });
            }
        }

@Telroshan walked me through the spec on the event and script ordering, and we thing this covers all our bases, and it encrapsulates the ugly logic within the function itself. It's ugly, but encrapsulated.

[1cg]

@ahollandECS thoughts on above?

[alexpetros]

It's genuinely insane that the variable you can check for the document state doesn't match up with the events that are fired.

Anyway, I support that solution, think the additional closure is worth it.

[ahollandECS]

I pushed an update, encapsulating everything inside ready() while trying to keep it legible. I think this still works for all the cases my previous version did, trading a bit of legibility for encapsulation.

[1cg]

beauty! thank you for your work on this!