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

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

Open
ohmyohmyohmy opened this issue Sep 5, 2024 · 7 comments
Open
Labels
bug Something isn't working

Comments

@ohmyohmyohmy
Copy link

ohmyohmyohmy commented Sep 5, 2024

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
Copy link
Contributor

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
Copy link
Author

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

@MichaelWest22
Copy link
Contributor

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 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
Copy link
Collaborator

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

  • 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!
  • 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)

@MichaelWest22
Copy link
Contributor

MichaelWest22 commented Sep 6, 2024

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
Copy link
Contributor

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

@MichaelWest22
Copy link
Contributor

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

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

3 participants