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

Ajax request sometimes triggered twice with hx-trigger="load" in web components #2973

Open
basvk opened this issue Oct 18, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@basvk
Copy link
Contributor

basvk commented Oct 18, 2024

I'm working with web components (with lit.dev) and using htmx to populate or replace them in the page. This saves me from writing a lot of business logic and networking code in the frontend. I'm not using shadow dom, and in general it works pretty well. I'm calling htmx.process inside the components on update.

Sometimes I'm seeing duplicate ajax requests occurring on (nested) elements with hx-trigger="load", mainly on Safari 17.6.
In my case the response is swapped in with hx-swap="afterbegin", causing duplicated children in the web component.
The element triggering the load is in a nested hierarchy. Top nodes do not show this behaviour.

Here-in lies the issue: in a regular web page htmx.process (or rather processNode) after initial page load is only called on the body, running initNode on all child elements with htmx attributes once.
With web components however, htmx.process is called for every web component, and every web component nested within it. This means children are visited multiple times, once for every parent component that calls htmx.process plus the child itself.
Luckily, this by itself is not a problem, because initNode checks if the node already has a corresponding initHash value and will not init again in that case.
However, the initHash is calculated from the node's attributes. Between the different calls to htmx.process from each webcomponent, the attributes sometimes do change, causing initHash to differ and to call initNode again on a node.

In my scenario between calls to htmx.process, the element (when visited during processNode on a parent element) with hx-trigger="load" starts an ajax request and gets class="htmx-request" set. This alters the attributes, meaning that the element's initHash is not correct anymore for the attributes. This results in the node being initialized again during the next call to htmx.process (on another parent or itself) and starting a second ajax request...

Now I've solved this particular case by adding a check to not initialize if the node has htmx-request in its classList (see this commit: master...basvk:htmx:fix-double-load - let me know if you'd like a pull request), but I'm not sure if there are other cases where multiple calls to htmx.process might cause these kind of issues.

Maybe initHash should be based on a subset of attributes, or calculated in a different way when dealing with web components? What is your opinion?

@Telroshan
Copy link
Collaborator

Interesting issue, in this case I think you might want to define a delay to postpone the load trigger, with for example hx-trigger="load delay:1ms", 1ms is already probably enough to get the load event out of the main queue and get all the children nodes processed before this event fires?

Though I agree that ideally you shouldn't need to do that, htmx should handle it itself by default.
I'm afraid removing the class attribute altogether from the current hashing logic could be a breaking change for existing apps.
At the same time, I don't know if adding an exception for htmx classes in the code is the right way to go? Maybe there's an easier way, but nothing comes to mind at the time of writing this.

I'd consider this as a bug, so feel free to explore whatever way feels best for you to fix the issue and submit a bugfix PR @basvk. Just keep in mind the usual constraints (no breaking change, add tests to prove the issue happens without your fix, then doesn't happen anymore with your fix)

If you go with the classlist approach you did in the commit you referenced, you'll likely want to handle other htmx classes (indicatorClass, requestClass, addedClass, etc.)

@Telroshan Telroshan added the bug Something isn't working label Oct 19, 2024
@basvk
Copy link
Contributor Author

basvk commented Oct 19, 2024

Adding the delay is a nice quick workaround for anyone hitting this issue, thanks!

For a more robust fix, maybe the hashing logic needs to be revised? It seems the value of initHash is only used in initNode (its existence is used in maybeReveal but not its exact value). Looking at what happens in initNode, only a limited set of element attributes have influence on its behavior. Any other attribute changing does not influence its result, so technically these could be ignored when calculating the attributesHash.

For as far as I can see these attributes are used by initNode (and functions called from within initNode):

  • [data-]hx-boost
  • [data-]hx-get/post/put/delete/patch
  • [data-]hx-target
  • [data-]hx-trigger
  • action (on elements i.c.w. hx-boost)
  • form (on button elements)
  • href (on <a> elements i.c.w. hx-boost)
  • method (on <form> elements i.c.w. hx-boost)
  • target (on <a> elements i.c.w. hx-boost)
  • type (on <input> and <form> elements)

If we base the calculation of the initHash on these attributes only, we forego unnecessary re-initialization when irrelevant attributes are changed between processNode calls (quite a normal occurrence when using web components).

Do you see any problems with this approach (other than the amount of code slightly increasing)?

@Telroshan
Copy link
Collaborator

You're right @basvk , the purpose of the hash is to determine whether an element's specs changed (in which case we should reprocess it), for example if its hx-trigger specs changed, but unused attributes won't have anything to do with it, including the class.

I like your approach, except the amount of hardcoded strings it's going to add 😆 I'm also a bit worried that this is the kind of function we may forget behind if/when adding new attributes in the future that should be hash-changing.
What do you think of keeping the current loop to iterate over all attributes, but only process attributes that starts with hx- or data-hx, as well as the list of standard attributes you mentioned above?

From the top of my head I think it's safe to just process all htmx-related attributes here (to ease the processing & avoid future omissions), and to do a simple prefix check as we have a very specific prefix for our attributes anyway, that would very unlikely collide with anyone else's code

Let me know!

@basvk
Copy link
Contributor Author

basvk commented Oct 20, 2024

If we limit the attributesHash to only hx- and data-hx attributes, changes to the action, form, href, method, target and type attributes on an existing element will not result in a change of the triggerHandler for that element anymore. This could impact users (example: existing hx-boosted <a> that gets its href changed will still execute an xmlHttpRequest to the previous href).

I took a step back and I'm realizing the real issue is that hx-trigger="load" should only be handled when an element has no initHash set at all (meaning this is the first time htmx ever sees it). If it has an initHash, and the attributes changed, it should not handle hx-trigger="load", because the element was already loaded.

Refining the attributeHash calculation actually does not fully fix that, if I change a hx- attribute on an existing element that has hx-trigger="load" (probably IRL something like hx-trigger="load, <some other triggers>", changing hx-vals for example), it will still execute a second XMLHttpRequest for the "load" trigger.

So I think I'll leave the attributeHash calculation as it is now (the performance benefit alone does not seem compelling enough to make any changes).
Instead I rather change the initNode a bit to handle hx-trigger="load" only when initHash is empty. Maybe by adding a flag in nodeData 'firstInit' that addTriggerHandler can read, that gets unset at the end of initNode?

@basvk
Copy link
Contributor Author

basvk commented Oct 20, 2024

I went ahead and created a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants