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

hx-swap-oob should support all hx-swap attributes #2308

Open
aral opened this issue Feb 12, 2024 · 18 comments
Open

hx-swap-oob should support all hx-swap attributes #2308

aral opened this issue Feb 12, 2024 · 18 comments

Comments

@aral
Copy link
Contributor

aral commented Feb 12, 2024

hx-swap-oob should support all hx-swap atttributes (swap, settle, delay, etc.) but fails with "is not a valid selector" error (support for this is not present currently in hx-swap-oob in the main branch or in v2.0v2.0).

e.g.,

<p hx-swap-oob='true settle:0.5s'></p>

Results in:

Uncaught DOMException: Document.querySelectorAll: '0.5s' is not a valid selector

This is an important missing use case for WebSocket workflows where you want to allow CSS transitions time to play between updating nodes.

Add this failing test to /test/attributes/hx-swap-oob.js to recreate:

  it('works with a swap delay', function(done) {
    this.server.respondWith('GET', '/test', "<div id='d2' hx-swap-oob='innerHTML swap:10ms'>Clicked!</div>")
    var div = make("<div id='d1' hx-get='/test'></div>")
    var div2 = make("<div id='d2'></div>")
    div.click()
    this.server.respond()
    div2.innerText.should.equal('')
    setTimeout(function() {
      div2.innerText.should.equal('Clicked!')
      done()
    }, 30)
  })

As far as I can see at the moment, the main issue appears to be that swap() calls oobSwap() whereas each out-of-band swap should really be calling a generic swap() function with a reference to the element to affect that applies the full set of swap functionality. Then regular swaps would become a special case where the element in question is also the one that triggered the event.

The only other way I can see of implementing this is to repeat the functionality that’s already in swap() in oobSwap() which isn’t ideal by any means.

Discovery

I’ll update this section as I look into this issue further with an eye towards implementing this functionality for hx-swap-oob in general and for the ws extension in particular (from a cursory glance, it looks like that requires work also to implement this).

  • getSwapSpecification() in htmx.js only reads values defined in hx-swap. This is an easy fix:

    function getSwapSpecification(elt, swapInfoOverride) {
      var swapInfo = swapInfoOverride ? swapInfoOverride : getClosestAttributeValue(elt, "hx-swap");
      if (!swapInfo) swapInfo = getClosestAttributeValue(elt, "hx-swap-oob");
        var swapSpec = {
          "swapStyle" : getInternalData(elt).boosted ? 'innerHTML' : htmx.config.defaultSwapStyle,
          //…
  • ws extension does not appear to handle swap/settle delays at all in the message event handler, unlike the logic that exists for doing this in htmx’s handleAjaxResponse(). Looks like we will have to duplicate a lot of the code in the latter to implement the functionality in the former. Given that, does it make sense to keep the WebSocket extension as an extension instead of having it in core? (Or, alternatively, should the doSwap() and doSettle() functions be exposed on the htmx API as these will be required to implement this functionality in both the SSE and WS extensions for consistency.)

    Update: Given that this functionality has now been encapsulated in the new swap() function in v2.0v2.0 ­– and this is a good step in the right direction – I guess what remains is to make sure that oobSwap() uses it too (see thoughts, above).

    This is more complicated and requires a design decision. Thoughts, @1cg?

@aral
Copy link
Contributor Author

aral commented Feb 13, 2024

I see that the current work appears to be under https://github.com/bigskysoftware/htmx/tree/v2.0v2.0 so I’m going to familirise myself with that branch and (a) see if the issue still exists there and (b) if so, possibly start attempting a fix there in my own fork.

@aral
Copy link
Contributor Author

aral commented Feb 13, 2024

Issue is still there in v2.0v2.0 branch.

e.g., failing test to add to test/attributes/hx-swap-oob.js to recreate:

  it('works with a swap delay', function(done) {
    this.server.respondWith('GET', '/test', "<div id='d2' hx-swap-oob='innerHTML swap:10ms'>Clicked!</div>")
    var div = make("<div id='d1' hx-get='/test'></div>")
    var div2 = make("<div id='d2'></div>")
    div.click()
    this.server.respond()
    div2.innerText.should.equal('')
    setTimeout(function() {
      div2.innerText.should.equal('Clicked!')
      done()
    }, 30)
  })

(Fails with error in Mocha tests when run in browser.)

@aral aral changed the title hx-swap-oob fails on hx-swap attributes that contain a colon hx-swap-oob should support all hx-swap attributes Feb 13, 2024
@alethiophile
Copy link

I'd like to +1 this. I'm currently working with the websocket extension, and I would really like to be able to do the beforeEnd scroll:bottom thing, but it's not possible with the current logic.

@aral
Copy link
Contributor Author

aral commented Feb 22, 2024

This also affects idiomorph’s morph: syntax (you can’t use that when using the WebSocket extension – interestingly, I was just trying to make a markdown preview updated via WebSocket scroll into view to show the latest change after the content has been morphed and I can’t).

Will open a separate issue for that under idiomorph and link it here.

@jeffturcotte
Copy link

I really wanted this too for v2 :) #2367

We'll see where the discussion goes. Thanks for the test @aral

@aral
Copy link
Contributor Author

aral commented Mar 2, 2024

@jeffturcotte Oh wow, you implemented it :) This is awesome! CC @1cg, have you seen this? Thoughts on getting it into v2? :)

@jeffturcotte
Copy link

PR was closed due to no discussion. This issue has the most detail, so makes sense to continue to track it here.

Here is the fork with working swap normalization and OOB modifier support: https://github.com/jeffturcotte/htmx/tree/normalize-swaps

Would love for someone to look at this as normalizing swap/oob-swap behavior feels like a no-brainer for v2! I personally ran into a lot of confusion when first encountering this issue.

My implementation is pretty close to the write-up by @aral, but differs in that swap() is not as universal as described. In order to best tip-toe around and keep the existing patterns/logic/API consistent, oobSwap is still independent of swap, but duplication has been minimized as a lot of the swap logic was abstracted to new methods.

From my PR:

Normalizes the new swap() api to handle all modifiers internally such as view transitions, swap delay, and settle delay. Previously these features were limited to the XHR handler and would not work via other contexts, such as SSE/WS.
Brings OOB swaps closer to being "first class swaps" as support for swap modifiers has been added to them.

An example of how modifiers can be used on oob swaps:

<div hx-swap-oob="true:.selector transition:true swap:3s settle:2s"></div>

I am not tied to my implementation by any means, but I wanted this functionality ASAP so ended up moving forward with it.

aral referenced this issue Mar 4, 2024
Was pulling my hair out trying to get this working before I realized
that it's a known issue.
@aral
Copy link
Contributor Author

aral commented Mar 4, 2024

@jeffturcotte This looks like a great first step toward the goal of making SSE and WebSockets first-class citizens and have hx-swap and hx-swap-oob behave consistently with the only difference being the -oob part of the latter (which is a reasonable expectation to have from the framework).

I’m going to have a play with your branch today and I might actually end up incorporating a fork of htmx with your work into Kitten as it feels like folks are otherwise busy and I’ve already delayed things about a month waiting to hear back about this and I really need to plod on if I’m ever going to release anything :)

@aral
Copy link
Contributor Author

aral commented Mar 15, 2024

@jeffturcotte I just started a discussion on the htmx Discord about this (https://discord.com/channels/725789699527933952/1046573806547910677) and @Renerick kindly responded that this would be good to try and get into 2.0

As such, I just rebased your branch into the latest v2.0v2.0 on my fork (and somehow lost your tests during – sorry – so had to add them back in in a separate commit).

You can find that branch here:

https://github.com/aral/htmx/tree/normalize-swaps

All tests are passing and this implements the most important missing functionality for using oob swaps in WebSocket and SSE-based apps.

Basically, unless I’m missing something, we should be able to just merge your branch and have this implemented in about as maintainable a way as we’re going to get without a huge rewrite to change the fundamental relationship between and design of swap() and swapOob().

Will ping @Renerick on Discord with a link to this too.

@jeffturcotte If you’d like to rebase your branch on top of the latest v2.0v2.0 branch, that would likely be the cleanest/easiest way to proceed.

And then we could perhaps reopen #2367 and look at merging it in?

Thoughts?

@aral
Copy link
Contributor Author

aral commented Mar 16, 2024

@jeffturcotte @Renerick Update to the above.

All tests were not passing with Jeff’s code for me, only the ones in the browser that I ran using npx serve from localhost:3000. Specifically, once I ran the tsc tests, I saw a number of failures.

(For some reason, I can’t run mocha-chrome properly from my computer with Chrome installed via Flatpak. It keeps failing with ECONNRESET).

In any case, I believe I’ve now handled the issues but I’d really appreciate the eyes of someone who knows the codebase better than I do on them.

Specifically:

  • The htmx-settling class was not being applied, fixed that in aral@4119106

  • The wrapWithTransition() function was referring to an unknown variable, elt. This was also the issue in the doSwap() closure. In both, I replaced it with target as I believe this is the intent (please correct me if I’m wrong). I also cast a couple of things to avoid type errors: aral@1c968f3

  • I added a fallback for swapObject being undefined in aral@073c47a but I’m not sure if this is a smell for a bigger issue or not.

@jeffturcotte
Copy link

@aral I should be able to get to the rebase later this week.

Maybe I was not running all the tests correctly? They were green on my end. 🤷

@aral
Copy link
Contributor Author

aral commented Mar 19, 2024

@aral I should be able to get to the rebase later this week.

Yay, thanks! Please also see my branch for fixes you might want to copy over.

Maybe I was not running all the tests correctly? They were green on my end. 🤷

I initially had the same issue. For some reason I can’t get mocha-chrome to work for me using the Flatpak of Chrome so I was only running the tests in the browser via npx serve and then hitting the tests URL instead of running npm test which would fail at the mocha-chrome stage. But that means that you also don’t run the TypeScript compiler and miss out on the static analysis, which is where the issues where surfaced.

You can run the compiler manually using:

tsc --project ./jsconfig.json

@aral
Copy link
Contributor Author

aral commented Mar 19, 2024

@jeffturcotte Also, please see 6d5e03f by @Renerick – that fixes hx-vals not being sent correctly for WebSocket calls in v2.0v2.0 just so we don’t miss it off of this PR.

@aral
Copy link
Contributor Author

aral commented Mar 19, 2024

@jeffturcotte Hey Jeff, just a quick head’s up, I’ve gone ahead an added a fork of htmx 2 and htmx-ws based on your work to Kitten along with a little shout out to you and Denis in the contributors doc.

PS. In case it helps, my forks:

Thanks again + looking forward to seeing this in the official branch (@1cg mentioned in Discord that it would be after the release of 2.0 as they’re very busy at the moment – hopefully towards the end of April or so) and switching to the official branch in Kitten too.

@RussBaz
Copy link

RussBaz commented Apr 25, 2024

Sorry for bothering, but is there a workaround for this if I am using 1.9.12?

I am using hx-swap-oob to lazy load a column on a table, and I would love to apply some CSS transitions to it.

@alethiophile
Copy link

The workaround I use is to catch the websocket events and do the stuff manually in JS.

@reedspool
Copy link

reedspool commented Jun 29, 2024

I am posting my generic workaround for the lack of support for hx-swap-oob="beforeend scroll:bottom" - I posted it and then realized this thread is about the way more general problem of all hx-swap attributes, but... maybe it'll help someone (and also bump this thread since v2 has been released).

<script>
  {
    const logSelector = "#update-log";
    const log = document.querySelector(logSelector);
    const main = document.querySelector("main");
    let isAtBottomOfScroll = false;

    main.addEventListener("htmx:oobBeforeSwap", (event) => {
      if (!event?.detail?.target === log) return;
      isAtBottomOfScroll =
        // We're locked if we're scrolled all the way to the bottom with some
        // constant for leeway/error
        log.scrollTop + log.getBoundingClientRect().height + 20 >=
        log.scrollHeight;
    });

    main.addEventListener("htmx:oobAfterSwap", (event) => {
      if (!event?.detail?.target === log) return;
      if (!isAtBottomOfScroll) return;
      log.scrollTop = log.scrollHeight;
    });

    window.addEventListener("DOMContentLoaded", (event) => {
      log.scrollTop = log.scrollHeight;
    });
  }
</script>

@RokLenarcic
Copy link

+1 on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants