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

Add config option to ignore nested oob-swaps instead of processing them #1235

Merged
merged 5 commits into from
Mar 11, 2024

Conversation

infogulch
Copy link
Contributor

@infogulch infogulch commented Feb 1, 2023

This adds a new configuration option, allowNestedOobSwaps, which preserves existing behavior when set to the default true, but when set to false it changes handleOutOfBandSwaps to only process elements with hx-swap-oob and data-hx-swap-oob that appear at the root of the response fragment, i.e. elements whose parent element is body. Elements deeper in the dom tree fragment are ignored and the oob-swap attribute is stripped instead.

The current system means you can just randomly intersperse oob fragments anywhere within the main response and they are swapped in from wherever to wherever. It seems much more conceptually sound to require all oob-swaps to originate adjacent to the response's main element.

Enabling this configuration option is helpful when you want to reuse a small html fragment to render both as as an element within a larger fragment (say a page) as well as more fine grained interactions where the small fragment is updated out of band. By setting allowNestedOobSwaps: false, you can place hx-swap-oob="true" on the fragment and leave it: when a full page renders then the attribute ignored since the fragment is nested inside the main response, but if you do want to update the fragment out of band then render it adjacent to the to the root response element and htmx performs the oob swap as expected. However in this scenario if allowNestedOobSwaps: true (default) then the small fragment is removed from the root element first and then the root element is swapped leaving you confused about why the small fragment vanished.

Here's an example of how this simplifies htmx usage in Go templates. In particular, note how this eliminates the conditional logic that controls whether hx-swap-oob="true" is included in the response or not: infogulch/go-htmx@ea227b6

This PR implements the feature request in #1133

@infogulch infogulch changed the title Ignore descendant oob-swaps instead of eating them. Fixes #1133 Ignore descendants' oob-swaps instead of eating them; fix #1133 Feb 2, 2023
@infogulch infogulch changed the title Ignore descendants' oob-swaps instead of eating them; fix #1133 Ignore descendants oob-swaps instead of eating them; fix #1133 Feb 2, 2023
@infogulch infogulch changed the base branch from master to dev February 22, 2023 23:38
src/htmx.js Outdated Show resolved Hide resolved
@alexpetros alexpetros added the bug Something isn't working label Jul 18, 2023
@alexpetros
Copy link
Collaborator

This looks good—could you add tests that validate the new functionality?

@alexpetros alexpetros added enhancement New feature or request and removed bug Something isn't working labels Jul 18, 2023
@infogulch
Copy link
Contributor Author

infogulch commented Jul 24, 2023

I added a test that should show the new behavior, but I'm not sure it's correct.

@infogulch infogulch force-pushed the patch-1 branch 2 times, most recently from 80285ea to 2bd14b4 Compare July 24, 2023 17:46
@infogulch
Copy link
Contributor Author

I updated the test so it should pass if it were to run again.

That said I'm not sure if the behavior is exactly what we want. Specifically, should the final innerHTML contain the hx-swap-oob attribute or should it be stripped? The point of this PR is to preserve the <span> in this scenario, but my question now is should it preserve the hx attribute?

        btn.innerHTML.should.equal('<button>Clicked <span id="when" hx-swap-oob="true">now!</span></button>');

@alexpetros
Copy link
Collaborator

alexpetros commented Jul 30, 2023

Specifically, should the final innerHTML contain the hx-swap-oob attribute or should it be stripped?

What happens for a successful OOB swap? Is the attribute there? It should match that behavior.

I updated the test so it should pass if it were to run again.

Unfortunately it does not pass, but you can run it locally with npm run test

Also, if you could update the swap-oob docs to explicitly say that elements with swap-oob not in the root are ignored that would be great.

@alexpetros alexpetros changed the title Ignore descendants oob-swaps instead of eating them; fix #1133 Ignore descendants oob-swaps instead of eating them Jul 31, 2023
@infogulch infogulch force-pushed the patch-1 branch 3 times, most recently from bfc9f5e to afd7b12 Compare August 22, 2023 16:54
@infogulch
Copy link
Contributor Author

infogulch commented Aug 22, 2023

Ok I looked into this further and this PR seems to be in tension with the 'oob swaps can be nested in content' test.

The question is: What to do when an HX response body contains a nested element with the hx-swap-oob attribute? (The case of subsequent root elements with the hx-swap-oob attr is already clear.)

  1. The status quo's answer is to remove them from their parent element and swap them into the document in parallel with the parent element.
    • The downside is that if you want the child element to stay with its parent then you have to strip off the hx-swap-oob before sending depending on if you send the parent and child or just the child element, and it's a bit of a pain to plumb conditionals everywhere.
  2. My conception, and this PR, would change that behavior to assume that the intention is to keep the nested element together with its parent and ignore the attribute. This lets you reuse nested fragments depending on the need and not have to worry about whether the inner element contains a hx-swap-oob attr.
    • The downside is that this changes current behavior.

IMO, 'oob swaps can be nested in content' seems kinda silly; if you want to do an oob swap then return it at the root, adjacent to the main response. But the current system means you can just randomly intersperse parts of the page anywhere within the main response and its just swapped in from wherever to wherever. It seems much more conceptually sound to require all oob-swaps to originate adjacent to the response's root element, and consider oob-swaps elsewhere to be superfluous and ignore them.

I wanted to surface this difference and get feedback before I proceed due to the behavior change. Thoughts?

@alexpetros alexpetros added the under discussion Issues that are being considered but require further alignment label Aug 22, 2023
@alexpetros
Copy link
Collaborator

It seems much more conceptually sound to require all oob-swaps to originate adjacent to the response's root element, and consider oob-swaps elsewhere to be superfluous and ignore them.

Very hard to know if anyone is relying on this behavior, though I agree with you that, conceptually, it's a little absurd.

@Itzdlg
Copy link

Itzdlg commented Sep 30, 2023

I will note that the documentation currently warns: “Out of band elements must be in the top level of the response, and not children of the top level elements.”

Perhaps adding another property like hx-swap-oob=“true with-depth” or a server response header HX-Swap-OOB-Depth: true? Just some way to either reverse new functionality (or to set new functionality to keep backwards compatibility)

@infogulch
Copy link
Contributor Author

This has been paused for a while, but I'm currently making this change as a configuration option to switch behaviors. A 2.0 release might consider changing the default for this configuration option.

@alexpetros
Copy link
Collaborator

Yeah I apologize about this being stalled, it's a tough one for the reasons identified. We're closing in on having many fewer PRs, which will make it easier to focus on the hard choices. I think a config is a good call.

@infogulch infogulch changed the base branch from dev to master February 12, 2024 21:27
@infogulch infogulch changed the title Ignore descendants oob-swaps instead of eating them Add config option to ignore descendants oob-swaps instead of eating them Feb 12, 2024
@infogulch infogulch force-pushed the patch-1 branch 2 times, most recently from bfc9f5e to a23fe23 Compare February 12, 2024 21:38
@infogulch infogulch force-pushed the patch-1 branch 3 times, most recently from 5638a73 to aa321e3 Compare February 12, 2024 22:31
@infogulch
Copy link
Contributor Author

infogulch commented Feb 12, 2024

With this change, the current behavior is preserved by default, and can be changed by adjusting the config option. Perhaps a later version could change the default and eventually remove the config option.

@infogulch infogulch changed the title Add config option to ignore descendants oob-swaps instead of eating them Add config option to ignore nested oob-swaps instead of processing them Feb 25, 2024
@infogulch
Copy link
Contributor Author

infogulch commented Feb 25, 2024

The description has been updated to add my motivation for opening this PR.

Just off the top of my head here are a couple things that I think would improve it:

  • Update config listing
  • Update docs to reflect how oob swaps work when the config is set to both settings

Should I continue working on this? What are htmx collaborators' disposition to this change?

@alexpetros
Copy link
Collaborator

alexpetros commented Feb 25, 2024

I like the config option and I'm going to push for it. I'll get you back a final up or down soon (< a week). Would probably be against the htmx 2 branch, if that's alright?

@alexpetros
Copy link
Collaborator

alexpetros commented Feb 25, 2024

A good reason to support this, which I think you're calling out, is that it composes really well with template fragments. You can send the fragment as an OOB swap, or just send the template normally.

@infogulch
Copy link
Contributor Author

A good reason to support this is that it composes really well with template fragments. You can send the fragment as an OOB swap, or just send the template normally.

Go ahead and just summarize my giant meandering paragraphs in two pithy sentences. It's fine. I'm not mad. Why would I be mad? I'm a great writer I'll have you know. 😡😆😭

@alexpetros
Copy link
Collaborator

alexpetros commented Feb 26, 2024

If you weren't a good writer I wouldn't have know what you were talking about ☺️ I have the advantage of not having to also explain all the technical details.

Anyway, got the go-ahead! Can you rebase again the 2.0 branch and add the docs?

@infogulch
Copy link
Contributor Author

Great! I'll get it done.

@infogulch infogulch changed the base branch from master to v2.0v2.0 February 27, 2024 00:21
@infogulch
Copy link
Contributor Author

infogulch commented Feb 27, 2024

  • Rebase changes onto v2.0v2.0 branch, assuming that's the right one
  • Update the configuration listings (both of them, thank you @infogulch for that)
  • Add a short explanation of the config option on the hx-swap-oob attribute page
  • Fix lint errors
  • Fix issues with parentNode === null
  • Add tests for htmx.config.allowNestedOobSwaps + template node
  • Add system to save/restore config between tests to test specific configurations
  • Duplicate various hx-swap-oob tests with different settings for htmx.config.allowNestedOobSwaps to validate that they work
  • Add tests for when the response is wrapped in html and/or body elements

@alexpetros
Copy link
Collaborator

@infogulch ready for review?

@infogulch
Copy link
Contributor Author

@alexpetros yessir

Copy link
Collaborator

@alexpetros alexpetros left a comment

Choose a reason for hiding this comment

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

Thanks for all your hard for on this PR, it's really excellent and we're excited to have it as a part of htmx 2 :)

@alexpetros alexpetros merged commit 5205021 into bigskysoftware:v2.0v2.0 Mar 11, 2024
1 check passed
@infogulch infogulch deleted the patch-1 branch March 11, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request under discussion Issues that are being considered but require further alignment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preserve elements with hx-swap-oob that are not at the root of a response
4 participants