Running ready function on readyState === 'interactive'

andrew-ebsco commented 7 months ago

Hello htmx team!

I have been working on a confusing bug that I was able to fix, but want to present it to the team before making a pull request. I have started using htmx in Magento for some custom features. Magento uses RequireJS to load AMD modules and htmx runs well in the set up. However, RequireJS loads all modules with async="" on all script tags. The bug I ran into is that the htmx initialization function was not running sometimes. Since the document body was not being processed, it required a page refresh to get the htmx behavior to execute. After some debugging, I found the issue was in how the ready function checks for a complete DOM.

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

/**
 * Execute a function now if DOMContentLoaded has fired, otherwise listen for it.
 *
 * This function uses isReady because there is no realiable way to ask the browswer whether
 * the DOMContentLoaded event has already been fired; there's a gap between DOMContentLoaded
 * firing and readystate=complete.
 */
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') {
        fn();
    } else {
        getDocument().addEventListener('DOMContentLoaded', fn);
    }
}

In the above code, there is a scenario where fn is never executed. Sometimes the htmx code was being executed after the DOMContentLoaded event had fired, but before document.readyState === 'complete'. After doing some research, I found that DOMContentLoaded can fire when document.readyState === 'interactive'. According to both MDN and the HTML Spec, the DOMContentLoaded event and a readyState of interactive are equivalent.

As a result, my proposal is to change the ready function to check for both a readyState of interactive as well as complete.

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

/**
 * Execute a function now if DOMContentLoaded has fired, otherwise listen for it.
 *
 * This function uses isReady because there is no realiable way to ask the browswer whether
 * the DOMContentLoaded event has already been fired; there's a gap between DOMContentLoaded
 * firing and readystate=complete.
 */
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 === 'interactive' || getDocument().readyState === 'complete') {
        fn();
    } else {
        getDocument().addEventListener('DOMContentLoaded', fn);
    }
}

The change allows fn to be immediately executed if DOMContentLoaded has already fired, but the readyState equals interactive. Since DOMContentLoaded and readyState === 'interactive' are functionally equivalent, this small change should fix the async bug I have encountered without introducing a regression. I have run the test suite locally and all tests passed without issue.

It looks like there was an attempt to fix loading bugs in PR 1972, but the code was more complex by watching the readystatechange event. It was eventually reverted in PR 2040 in favor of creating a new initialize function. That initialization function has not been created yet and as a result the async bug persists.

I believe my proposal is a much simpler fix, is inline with the HTML spec and maintains htmx's easy to install behavior. Of course, there are edge cases I don't know about. If this change would cause problems, I would like to hear about them. However, if the htmx team likes the fix, I can submit a pull request.

Thanks!

Telroshan commented 6 months ago

@alexpetros the always burning topic! As for your proposal, the issue with the interactive state is that, if I recall correctly, it was causing some issues with module imports (see #1672) But you're right, it seems we forgot about this initialization function that we had talked about 🤔

alexpetros commented 6 months ago

First of all, this is a very well-written issue that engages with all the past discussion and I just want to publicly appreciate that!

Magento uses RequireJS to load AMD modules and htmx runs well in the set up. However, RequireJS loads all modules with async="" on all script tags.

This is the wrong way to load htmx. Full stop. We do our absolute best to accommodate it, but it is a lower priority than all functionality that is supported by using htmx when loaded correctly, via either a script tag or a ES6 module. And as you noted, we have caused regressions in the past overstretching on our ability to support asnyc loading. So my first question is—why do you need to load htmx with Magneto? Why can't you just include htmx as a separate script tag in your document?

According to both MDN and the HTML Spec, the DOMContentLoaded event and a readyState of interactive are equivalent.

In practice, this does not seem to be exactly the case. If this were airtight, we could just say "if this readystate, listen for this event, otherwise activate." The code as is works because if you load htmx correctly, then the document will not be in a completed readyState without having read the code that adds the event listener.

All this having been said, I don't oppose this change. It's easy to understand and causes no harm and it would solve your problem. I still wouldn't say that we "support" async script tag loading though.

ryanisn commented 5 months ago

@andrew-ebsco did you try loading htmx into magento directly via script tag? when I do that it, console returns 'Uncaught Error: Mismatched anonymous define() module'

AlexanderArvidsson commented 1 month ago

I stumbled across this comment in the HTMX codebase (also pasted in this issue):

  /**
   * Execute a function now if DOMContentLoaded has fired, otherwise listen for it.
   *
   * This function uses isReady because there is no reliable way to ask the browser whether
   * the DOMContentLoaded event has already been fired; there's a gap between DOMContentLoaded
   * firing and readystate=complete.
   */
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 MDN and the HTML Spec is correct, then looking for "interactive" means there won't be a gap between DOMContentLoaded and readyState anymore, correct?

This is just a thought, but we should be able to remove the DOMContentLoaded listener that sets isReady = true and the isReady check completely, only relying on readyState > "interactive" (obviously not >, but you get the point), else rely on DOMContentLoaded within the ready function (as is done today). This is also very clearly clarified in this section on MDN about it having no race-conditions.

Although, this would revert #1672 to the readyState != "loaded" check, I'm not sure why listening for "interactive" would be too late (as mentioned in the PR), as "interactive" should come before "loaded".

On another note:

My use-case is a bit special, I need to initialize HTMX manually before DOMContentLoaded, as I am streaming data in. The entire page is first streamed as a whole, and then later additional data is streamed down in the same request into a shadow DOM (which is then later merged into the main DOM). HTMX will only initialize when the stream is completed, so any interaction before the stream is done will not work.

If I can manually initialize HTMX via a initialize function, I could stream down a script tag after the initial page which initializes HTMX. I wrote my own initialize function inside the HTMX library to test this, and it seems to be working great. But an official initialize method would make this a lot better.

Here's my rough suggestion:

  function initialize() {
    triggerEvent(getDocument(), "htmx:ready", {});
  }

  getDocument().addEventListener("DOMContentLoaded", initialize);

  htmx.initialize = initialize;

  function once(fn) {
    var executed = false;
    return (...args) => {
      !executed && fn(...args);
      executed = true;
    };
  }

  /**
   * Execute a function now if document is interactive, otherwise listen for `htmx:ready`.
   */
  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 (getDocument().readyState !== "loading") {
      once(fn)();
    } else {
      getDocument().addEventListener("htmx:ready", once(fn));
    }
  }

htmx:ready differs from htmx:load due to the use of setTimeout:

  ready(function () {
    ...
    getWindow().setTimeout(function () {
      triggerEvent(body, "htmx:load", {}); // give ready handlers a chance to load up before firing this event
      body = null; // kill reference for gc
    }, 0)
  });

once is there to avoid double-init, in the case for me when I need to initialize before stream completes, and HTMX would re-initialize again on DOMContentLoaded.

alexpetros commented 4 weeks ago

If MDN and the HTML Spec is correct, then looking for "interactive" means there won't be a gap between DOMContentLoaded and readyState anymore, correct?

All I can say about this is I've tried multiple times and what we have right now seems to be working for the most number of people, relative to anything we've tried before. Every time I come up with a theoretically airtight way to initialize the attributes, someone's use-case breaks it.

If I can manually initialize HTMX via a initialize function, I could stream down a script tag after the initial page which initializes HTMX.

I know this has been discussed before, but I can't find it immediately. Possibly @xhaggi had a version of this? I'm open to it, but not causing regressions in our initialization logic is priority 1.

kuafucode commented 4 weeks ago

My experience is I have to always modify htmx code to cover interactive event if I want to boost a magento page. If this may break the projects of other people, maybe we can at least make this configurable via meta tag?

alexpetros commented 4 weeks ago

Could you share exactly how you modify it? That might help us come up with a solution.

kuafucode commented 4 weeks ago

@alexpetros it is the exact same code modification that @andrew-ebsco suggested in this post


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 === 'interactive' || getDocument().readyState === 'complete') {
        fn();
    } else {
        getDocument().addEventListener('DOMContentLoaded', fn);
    }
}

so maybe we can add a config htmx.config.boostOnInteractive with default value false

alexpetros commented 4 weeks ago

Reading back over my original comment in Februrary:

All this having been said, I don't oppose this change. It's easy to understand and causes no harm and it would solve your problem. I still wouldn't say that we "support" async script tag loading though.

So yeah, if you want to PR that one-line change specifically, go for it.

AlexanderArvidsson commented 4 weeks ago

Reading back over my original comment in Februrary:

All this having been said, I don't oppose this change. It's easy to understand and causes no harm and it would solve your problem. I still wouldn't say that we "support" async script tag loading though.

So yeah, if you want to PR that one-line change specifically, go for it.

In my case, that one-line change is not enough to initialize HTMX in a streamed page environment, as even interactive DOMContentLoaded only runs when the page has finished streaming. A manual initializer, like the one from my previous comment, would be awesome in addition to the one-line change. It opens up for earlier initialization and is something necessary if you want to initialize HTMX before the HTTP stream has been completed. Of course, only if it doesn't case regressions.

For reference on why this is needed; see NextJS Partial Prerendering and Remix streaming. Disclaimer: I'm not using HTMX with Next or Remix, I am building my own framework which supports partial prerendering by streaming, and HTMX won't initialize until after the stream is done, meaning nothing is interactive via HTMX and clicking links and buttons just ignores HTMX tags until initialize.

andrew-ebsco commented 4 weeks ago

@alexpetros and @kuafucode, apologies, I dropped the ball on creating a PR for this. We modified our copy of htmx with the change which solved our immediate need. The positive news is it resolved our issue and has been working well with Magento for several months. I can submit a PR today, if you haven't already.

@alexpetros, to answer your question, I did try loading htmx as a separate script tag, but it caused another error. I'm guessing it was an issue with some scripts loading in RequireJS and some not.

andrew-ebsco commented 4 weeks ago

Just submitted it: #2808. Thanks everyone!

xhaggi commented 4 weeks ago

I have done some testing for the common methods to load htmx. Only with async loading (<script asyc>), htmx is not initialized correctly with the current implementation. I therefore assume that @andrew-ebsco PR should solve the problem.

`<script>`

ready() called -> readyState: loading, isReady: false
event "readystatechange" -> interactive
event "DOMContentLoaded"
init htmx
event "readystatechange" -> complete

`<script defer>`

ready() called -> readyState: interactive, isReady: false
event "DOMContentLoaded"
init htmx
event "readystatechange" -> complete

`<script async>`

ready() called -> readyState: interactive, isReady: false
event "readystatechange" -> complete

`<script type="module">`

ready() called -> readyState: interactive, isReady: false
event "DOMContentLoaded"
init htmx
event "readystatechange" -> complete

Dynamic loading of `<script>`

ready() called -> readyState: complete, isReady: false
init htmx

Telroshan commented 4 weeks ago

I think the issue with the initial readystate check (i.e. !== 'loading' vs === 'complete', as the latest PR above reverts the ready state check to what it was initially) was with extensions, see #1485 and #1469 So does this usecase now work with the latest PR @xhaggi ? (Asking you as you were the one who made that PR I just linked)

xhaggi commented 3 weeks ago

You`re right @Telroshan, forgot about that.

My comment https://github.com/bigskysoftware/htmx/issues/1469#issuecomment-1579033922 explaining the issue with using extensions when htmx is used in a module environment.

I think we need some further investigations.

xhaggi commented 3 weeks ago

BTW some time ago I created a PR #1681 that would fix this issue, but was rejected by @alexpetros in favor of the current implementation.

Telroshan commented 3 weeks ago

Yeah there have been multiple PRs and issues about this topic over time: PRs:

1485
1659
1672
1681
1688
1972
2037
2040
2808

Issues:

1469
1655
2033
2264

AlexanderArvidsson commented 3 weeks ago

@Telroshan Would you prefer a discussion about the ability to do early initialization for streamed pages in a separate issue? I feel it's related, but kind of overlooked in this discussion and may be better suited as a separate issue.

None of the issues listed talk about this (at least no conclusion about it has been made), but considering many modern JS frameworks are moving towards streamed pages (especially now that Lambda supports it), any framework wanting to do the same approach wouldn't be able to reliably use HTMX for it (which is my case).

alexpetros commented 3 weeks ago

@AlexanderArvidsson yes, that should be a separate issue, but I will say that my initial reaction is that htmx is a framework intended to optimize large-grain hypermedia requests—streaming pages isn't really in the scope of what we do. If it's a thing we can support without adding too much complexity to the core, there might be a route. Either way, it's a separate issue.

andrew-ebsco commented 3 weeks ago

@alexpetros @Telroshan Considering the potential for causing loading issues, sounds like the initialize function is what is needed, so users can decide when they want to htmx to load in the page load cycle. Do I need to close the associated PR? Any ideas around how that should be implemented? I could take a stab at it when time permits.

Telroshan commented 3 weeks ago

Hey @andrew-ebsco , appreciate that you're taking time to look into this. I must admit I'm a bit confused by how we did a full cycle on this topic again. Looking at the latest decisions:

We had come to a first agreement with @alexpetros and @1cg on a relatively complex way to handle all situations
Jokes on us, one case was not handled, then we figured out with @alexpetros that it was getting way too much of a mess to handle all those things, and it was not reasonable to keep going this way
The above PR was reverted in #2040 by @alexpetros, where we had stated

Another PR for initialize to come.

So it turns out, we completely forgot that initialize function (likely with htmx 2 work, I can't remember tbh) but as far as I know, that's what we were going for.

Though, this was in november 2023, time has passed, and minds can change. Context has been spread over many PRs and issues, and some cases / decisions might also have been forgotten along the way.

So, I'll think we'll have to dive into this whole topic with @alexpetros @1cg again, to see if we still agree with our past selves, or if we should try something else.

As for your PR @andrew-ebsco, I'd say it doesn't make sense to merge it in its current state in my opinion, as, as I commented on it, this would take us back to the very first loading check we had in the lib, that we already know is going to cause issues, as those very issues have been reported in the past (cf all issues and PRs listed above)

If you want to give a shot at the initialize function, go ahead! Note that #1485 might be a good inspiration, as it was a PR precisely about adding a way to manually initialize the lib.

AlexanderArvidsson commented 3 weeks ago

@Telroshan This is similar to what I was thinking as well.

I want to clarify my issue mentioned previously about streamed pages; HTMX works fine for streamed pages today, it's just that it won't initialize until the page is fully streamed in. This is separate from HTMX-initialized requests that might not support streams (which is fine and what @alexpetros said might not be in scope for HTMX) and I suspect that is how my issue was understood as separate, which is not necessarily what I'm asking for.

All that is needed to make server-initialized streamed pages work with HTMX is a way to do early initialization via an initialize function, which as @Telroshan says is what was forgotten in the past and that the recent PR suggested will do a full circle back to the very first check. Hence, if we can get a manual initialization, which will ultimately be useful for anyone encountering initialization problems (an escape-hatch), that's probably the way to go and we haven't gone back in a full circle :)

bigskysoftware / htmx