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

loading states: Respect path filter in processing undo callbacks #2297

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

rekado
Copy link

@rekado rekado commented Feb 9, 2024

Description

The loading-states extension queues up callbacks on htmx:beforeRequest. On htmx:beforeOnLoad it executes any undo callbacks that mayProcessUndoCallback allows. mayProcessUndoCallback only checks for the existence of the target.

Suppose we have multiple requests in flight. We use data-loading-path to only have matching requests modify the loading states of target elements. While attaching loading states can be filtered, removing loading states always succeeds as the undo queue is drained on htmx:beforeOnLoad.

This means that target elements are marked as loaded even though their requests are still in flight.
This change passes the request path to mayProcessUndoCallback to allow for delayed execution of undo callbacks when a path filter is available.

Corresponding issue: #2296

Testing

I tested this manually with a test page that would trigger multiple requests, served by a backend that introduces random delays to the responses. I used the loading states extension and only set up data-loading-aria-busy on target elements. With this change those elements whose requests were still in flight would remain in aria-busy state, while those whose requests were completed would have the attribute removed.

Without the change any completed request would cause the removal of aria-busy from all elements, even if their requests were still in flight.

Checklist

  • I have read the contribution guidelines
  • I have targeted this PR against the correct branch (master for website changes, dev for
    source changes)
  • This is either a bugfix, a documentation update, or a new feature that has been explicitly
    approved via an issue
  • I ran the test suite locally (npm run test) and verified that it succeeded
  702 passing (4s)
  1 pending

@rekado rekado changed the base branch from master to dev February 9, 2024 15:09
When a loading states path filter exists, respect it when deciding
whether to process the associated undo callback.
@Telroshan Telroshan added bug Something isn't working extension Consideration for an extension labels Mar 3, 2024
@davidjr82
Copy link

I had this bug and this PR fixed it.
In my opinion, it should be merged.

Thanks @rekado!

@rekado
Copy link
Author

rekado commented May 13, 2024

Can I do anything to increase the likelihood of getting this merged?

@Telroshan
Copy link
Collaborator

Hey @rekado, PRs are a bit stalled at the moment as we're focused on htmx 2, we'll get to PRs (and issues) again once htmx 2 is out.
Please note that with htmx 2, extensions will move to another repo, thus I'd encourage you to port your PR to this repo when you have the chance!
If you'd like to get this change into htmx 1, please retarget to the v1 branch as the dev branch now reflects htmx 2 which doesn't include extensions in its src anymore

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

Successfully merging this pull request may close these issues.

None yet

3 participants