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

Do not initialize htmx automatically on DOM ready when loaded as module #1485

Closed
wants to merge 2 commits into from

Conversation

xhaggi
Copy link
Contributor

@xhaggi xhaggi commented Jun 7, 2023

As discussed in #1469, when using htmx in a module environment, some internal extensions, such as preload, do not work properly. This is because htmx initializes itself on DOM ready, while the extensions become available later in the lifecycle. Therefore, it is necessary to disable the auto-initialization of htmx in these environments and expose a new API function htmx.start() to initialize htmx manually.

import htmx from 'htmx.org';
import <extensions>

htmx.start();

Related Issue: #1469

@xhaggi
Copy link
Contributor Author

xhaggi commented Jun 7, 2023

@matteocontrini please give it a try. You can use my Repo+Branch in npm with the following command.

npm i htmx.org@"git+https://github.com/xhaggi/htmx#fix/ext-async-loading"

src/htmx.js Outdated

// do not initialize htmx automatically if it is executed in module scope
// or the script element has the attribute async or defer
if (document.currentScript && !document.currentScript.async && !document.currentScript.defer) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not make this a param in the config that defaults to true, instead of that defer/async attributes check ?
You would keep the ability to disable htmx auto start when it becomes an issue, but I'd argue that since it's an issue in some cases, the auto initialization should be the default
So the behaviour wouldn't change for existing apps using htmx as defer/async (that would otherwise have to add an extra start call after upgrading)

Copy link
Contributor Author

@xhaggi xhaggi Jun 7, 2023

Choose a reason for hiding this comment

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

This is always an issue in all of the three cases (module, defer and async) if you use extensions that listen for events fired during the initialization phase of htmx. So we cannot rely on DOM ready in this scenario to initialize htmx.

Check out this small example:

https://jsfiddle.net/65au20sz/

Open your browser console (Level "Verbose" must be selected) and check the events logged with defer attribute and without. You should see something like this.

With defer attribute:

htmx:load

Without defer attribute:

htmx:beforeProcessNode
htmx:afterProcessNode
htmx:load

So why not make this a config setting? Because calling htmx.start() is much less complex as setting a config value via <meta name="htmx-config" content='{ "autoStart": true/false }'>. BTW you can't set a config setting in javascript, because htmx would be initialized right after importing htmx.org because the DOM is ready.

There are several approaches to initialize htmx after this is merged if you are in one of the three cases.

  1. Module (already described above)
import htmx from 'htmx.org';
import <extensions>
htmx.start();
  1. Script (defer / async) with a custom.js calling htmx.start()
<script src="htmx.js" defer></script>
<script src="<extensions>" defer></script>
<script src="custom.js" defer></script>
  1. Script (defer / async) with onload on last extension script
<script src="htmx.js" defer></script>
<script src="<extensions>" defer onload="htmx.start()"></script>

4 Script (defer / async) without extensions

<script src="htmx.js" defer onload="htmx.start()"></script>

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see the issue, thanks for the example.
While I fully agree for the module part, I'm still not fond of having to change/add something to an existing setup to have it working after the upgrade, for the defer/async part.

I would disagree on "much less complex", since it seems more intuitive to me to check the docs for a parameter to disable htmx auto initialization (which is a behaviour I'd expect when importing any similar lib), rather than wondering why does the lib suddenly not work anymore when I set it as defer/async, especially given that deferring your scripts avoids blocking rendering and improve page load times

In that case, I'd say the most common usage would be to use htmx without any extension, deferring it because you follow Lighthouse (or any other tool)'s recommandations to improve page load times, yet with this change, htmx wouldn't work anymore, and you'd have to wonder why.

Indeed, you would have here to use a meta tag to set that parameter in the config, since, as you said, the config object cannot be used before htmx gets loaded, but as the htmx doc says anyway,

Note that using a meta tag is the preferred mechanism for setting these properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your point, so I have rewritten the entire code to add a config setting htmx.config.autoStart. To disable autostart when used as a module, I changed the UMD wrapper and set the property htmx.config.autoStart = false. This way it is not necessary to check the document.currentScript property.

I have added another commit that I would like to discuss. The idea is to wait until all external extensions scripts are loaded before initializing htmx when defer or async is used. Therefore you need to mark all of the extension script elements with an attribute hx-ext (not really sure, if we should re-use this attribute). In the initialization phase of htmx, htmx now waits for the onload events of all extension script elements to fire and then initializes itself.

<script src="htmx.js" defer></script>
<script src="ext/preload.js" defer hx-ext></script>
<script src="ext/debug.js" defer hx-ext></script>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the effort!

That scripts attribute system would work indeed, and wouldn't cause retro compatibility issues here because of the change to add to existing setups, since it wasn't working properly anyway

I was wondering though, when looking at the htmx code, there is this setTimeout that lets any other listener bound to DOMContentLoaded, do their logic before the htmx:load event gets fired

Couldn't we apply the same logic here before processing the body ?
This would make the following chain call

  1. htmx executes its ready callback, merging configs with the meta tags, setups the htmx:abort listener, the onpopstate listener
  2. call setTimeout with a delay of 0, so that the function returns here, and the rest of the code executes later
  3. All other listeners bound to DOMContentLoaded in the document get called & processed. Here, extensions could bind themselves to htmx, after htmx has been initialized, but before htmx has processed the body
  4. the setTimeout callback gets called right after, htmx processes the body

Do you think that would work? From the top of my head, I can't think of any reason it wouldn't, but I may be missing something here.
The advantage of that solution (given that it works), would be that you wouldn't even need to add the hx-ext attribute to extension scripts, nor wait for all of them to initialize before auto-starting htmx. If I get it correctly, it would work by itself, automatically. But again, I may be missing something since I haven't tried out what I'm suggesting here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea and gave it a try. It only works for defer, but as you said, this is the preferred way to defer your scripts. In the few cases where you use async, you will need to adjust the configuration setting. I did a force-push to adjust the code.

@matteocontrini
Copy link

@xhaggi confirming it works.

I think the htmx import should be import * as htmx from 'htmx.org' and not import htmx from 'htmx.org', though.

@xhaggi
Copy link
Contributor Author

xhaggi commented Jun 8, 2023

Thank you @matteocontrini.

I think the htmx import should be import * as htmx from 'htmx.org' and not import htmx from 'htmx.org', though.

Why do you think we need a namespace import here?

import htmx, * as htmx2 from 'htmx.org'
console.log(htmx === htmx2) // --> true

@matteocontrini
Copy link

matteocontrini commented Jun 8, 2023

@xhaggi it might be that typings are incorrect but there is no default export:

https://github.com/bigskysoftware/htmx/blob/34612f0d0cfcd631d145a308eef5e079a5a53181/src/htmx.d.ts

So TypeScript complains if you use the default import syntax.

If it works with plain JavaScript, I guess typings are wrong.

@xhaggi xhaggi force-pushed the fix/ext-async-loading branch 2 times, most recently from f454f80 to 388822e Compare June 8, 2023 09:29
@xhaggi
Copy link
Contributor Author

xhaggi commented Jun 8, 2023

@matteocontrini I see what you mean. When I use your sample project in VSCode everything works. Compiling the main.ts and Vite dev/build also works. Am I missing anything to reproduce the problem?

import htmx from 'htmx.org';
import 'htmx.org/dist/ext/preload.js';
import 'htmx.org/dist/ext/debug.js';

htmx.start();

@xhaggi xhaggi changed the title Do not initialize htmx automatically on DOM ready when loaded as module, async or defer Do not initialize htmx automatically on DOM ready when loaded as module Jun 8, 2023
@xhaggi xhaggi force-pushed the fix/ext-async-loading branch 3 times, most recently from 90493e8 to 28f74f3 Compare June 8, 2023 10:40
@matteocontrini
Copy link

matteocontrini commented Jun 8, 2023

@xhaggi ah yes that's because that project uses esnext as the module syntax (see tsconfig.json), which automatically enables the esModuleInterop option that makes the default import syntax work.

If you change the module system to commonjs (the default) it won't work. So yeah, it depends on the setup, but import * as htmx from "htmx" should work always, I think.

@xhaggi xhaggi force-pushed the fix/ext-async-loading branch 2 times, most recently from cf22328 to 18f1c46 Compare June 8, 2023 13:51
@xhaggi
Copy link
Contributor Author

xhaggi commented Jun 9, 2023

@matteocontrini you are right and thank you for pointing me to the right direction. I wonder why there is a TypeScript test file src/htmx.test.ts where the default import is also used. When you call npx tsc --noEmit src/htmx.test.ts you get the error Module has not default export. Without the .d.ts you can use a default import and it works, also in your sample project. So it might be a problem with the type declaration, but that is out of scope of this PR.

@guilleiguaran
Copy link

I've tested and confirmed that this is working fine when using htmx and extensions with the native ESM support in the browser (without any JS bundler).

@alexpetros alexpetros added the enhancement New feature or request label Jul 29, 2023
@alexpetros
Copy link
Collaborator

alexpetros commented Jul 29, 2023

Marking this as an enhancement due to possible regressions. Will get this looked at soon, but in the meantime do you mind rebasing @xhaggi? Also, we don't edit the dist files directly, just the source.

@xhaggi
Copy link
Contributor Author

xhaggi commented Aug 7, 2023

@alexpetros done.

Also, we don't edit the dist files directly, just the source.

I know, and those changes were part of a commit that was marked to be skipped before the merge. I dropped that commit now.

@alexpetros
Copy link
Collaborator

@xhaggi can we close this PR since we merged this one? #1659

@xhaggi
Copy link
Contributor Author

xhaggi commented Aug 8, 2023

Sure. I can confirm that it works with the other fix.

@xhaggi xhaggi closed this Aug 8, 2023
@xhaggi xhaggi deleted the fix/ext-async-loading branch August 25, 2023 06:54
@xhaggi xhaggi mentioned this pull request Nov 28, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants