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.

Sign up for GitHub

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

Checking readyState is interactive before listening for DOMContentLoaded #2808

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Checking readyState is interactive before listening for DOMContentLoaded #2808

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/htmx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4899,7 +4899,7 @@ var htmx = (function() {
function ready(fn) {
// Checking readyState here is a failsafe in case the htmx script tag entered the DOM by
// some means other than the initial page load.
if (isReady || getDocument().readyState === 'complete') {
if (isReady || getDocument().readyState === 'interactive' || getDocument().readyState === 'complete') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we simply check readyState !== 'loading' in this case?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've tried that—I know this looks a little clunky but I'd rather both checks be included.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've tried that

Sorry but this confuses me @alexpetros , are you suggesting this might result in a different behavior than !== 'loading'?
As it's a 3 values property, this should be the exact same behavior, except that this adds more hardcoded strings not to be minified

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't read your suggestion closely enough (I missed the !==, that's my bad). Yes @andrew-ebsco can you make that change?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexpetros just to emphasize on this, this would take us back to august 2023, aka the first time we started dealing with these ready state / load event issues.
See #1672 which was the first one about this matter ; we precisely had a !== 'loading' check, exactly what we would go back to with this PR.
See my comment in #2264 with the history of PRs and issues related to this topic.

I'm not sure that's really what we should go for here, as we're getting back to a previous state that we already know is going to cause issues, as those very issues have been reported in the past (again, cf the issues and PRs listed in my comment, linked above)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexpetros just to emphasize on this, this would take us back to august 2023, aka the first time we started dealing with these ready state / load event issues.

I don't think is true, because what we have now that we didn't august 2023 is a variable that captures whether DOMContentLoaded fired.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm not sure this variable is useful anymore if we roll back to a !== 'loading' check @alexpetros

When you added that ready variable in #1688, it was precisely because we were only checking === 'complete', thus we could miss the timeframe between interactive and complete state, where we would bind to DOMContentLoaded while the document was already interactive, thus the listener would never be called.

If we roll back to a !== 'loading' check, I don't think there's any gap between the capture you linked above, and the first ready call of the lib.
Between this

htmx/src/htmx.js

Lines 4887 to 4890 in 51807d0

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

And this

htmx/src/htmx.js

Lines 4938 to 4939 in 51807d0

// initialize the document
ready(function() {

No async work is performed, so the DOMContentLoaded listeners would be bound one after the other, in the same frame.

So either the DOMContentLoaded event hasn't been fired yet, and both the capture and ready will bind a listener to it, making the capture useless here
Or, the event has already fired when htmx loads, so ready executes right away (as the state is interactive and no more loading), making the capture also useless here.

As per my comment in #2264 I really think we should take some time to dive in this topic again @alexpetros with @1cg as well, to see if A) we still agree with our past selves about the init function, and B) try to setup reproducible cases to test the different loading scenarios

fn()
} else {
getDocument().addEventListener('DOMContentLoaded', fn)
Expand Down