bigskysoftware / htmx

</> htmx - high power tools for HTML
https://htmx.org
Other
38.27k stars 1.31k forks source link

htmx.ajax swaps body's innerHTML when the target id doesn't exist #2869

Open ohmyohmyohmy opened 2 months ago

ohmyohmyohmy commented 2 months ago

I have an app where tabs hide certain div-ids when not loaded. When the div with the target id is visible, htmx.ajax() POST works as expected swapping the innerHTML as directed. But when the div with the id is no longer visible, the function replaces the entire innerHTML of the body with the response. Is this what's supposed to happen?

I'm using htmx 2.0.2

MichaelWest22 commented 2 months ago

This is happening because the htmx.ajax() ajax helper function is calling the the issueAjaxRequest function with the default body as the element and passing in a targetOverride set to the resolved target you passed in. If the targetOveride passed in is null because the resolved target is not found then it falls back to checking the body element for its target. And if you happen to have boosted=true on the body then it makes the body target the body causing your issue.

I think this is an edge case bug and not the intended behaviour.

to resolve it in your situation i would do:

if(htmx.find(target)) {
  htmx.ajax('GET', url, target)
}
ohmyohmyohmy commented 2 months ago

I didn't think of that. Thank you for that suggestion.

MichaelWest22 commented 2 months ago

Actually looking at it the boosted=true is not required as if target is invalid it falls back to checking body target and unless you set a hx-target on the body it will always return itself as the target boost or no boost. I don't know if this is really intended and may just be what it falls back to. Can fix the issue with https://github.com/bigskysoftware/htmx/compare/dev...MichaelWest22:htmx:ajax-no-target which gets it to htmx:targetError on bad requests but this could be a breaking change if someone was depending on this broken fallback behaviour?

Telroshan commented 2 months ago

this could be a breaking change if someone was depending on this broken fallback behaviour

It could but honestly it looks really convoluted to me, why would you want the server's reponse that you intended to swap at a very specific place (since you passed in a selector/target element), to instead replace the whole body's content? Imho it should reflect the behavior of hx-target, aka that if no target is found, the request doesn't happen and a htmx:targetError is fired. And as you did in your commit, only do that if a target override was explicitly set.

I like your suggested change @MichaelWest22 , feel free to open a bugfix PR. Just a few questions/suggestions though

MichaelWest22 commented 2 months ago

I like your suggested change @MichaelWest22 , feel free to open a bugfix PR. Just a few questions/suggestions though

  • Couldn't this be greatly simplified by removing asElement(getTarget(resolveTarget(context.source)))) || DUMMY_ELT altogether ? I would expect htmx to precisely fallback to the source element's hx-target value, I don't think this is needed, but let me know!

Yeah i've simplified this a bit now but it has to be a little complex sorry to handle all the situations I was testing it with. The logic is now:

 targetOverride: resolveTarget(context.target) || ((!context.target && resolveTarget(context.source)) ? null : DUMMY_ELT)

If resolveTarget(context.target) is not null|undefiend use this as the target Else if target is not supplied AND source resolves to a target then set target to null so that the valid source can be used for target checking only Else set target to DUMMY_ELT to trigger the target error.

  • You also might want to replace the falsy checks by explicit checks against undefined (or null). If you pass in an element reference directly that is null (for ex htmx.ajax('GET', '/test', { source: someElement, target: document.getElementById('whatever') } where whatever doesn't exist in the DOM), would you also consider this needs to fail like the other cases? Right now, I think your falsy checks will still let it fallback to the body. If you also think this should fail, I would suggest checking explicitly against null vs undefined (let undefined fallback to body as the param wasn't set at all, but null would mean it's a getElementById/querySelector that didn't resolve to the expected element)

I've updated my logic to make it a little more obvious what it is checking and I think its working well. But please review my updated logic to double check. Added tests already for most of the situations you mention.

MichaelWest22 commented 1 month ago

rewrote the logic into an expanded if statement so its easier to understand and debug and added inline comment

MichaelWest22 commented 1 month ago

@ohmyohmyohmy fix now in v2.0.3 so please retest and close if it is now resolved