Skip to content

Conversation

xhaggi
Copy link
Contributor

@xhaggi xhaggi commented May 29, 2024

Description

This adds three ways to cancel an hx-trigger polling. The first is a new configuration option htmx.config.cancelPollingOnError, which can be activated to cancel an hx-trigger polling in case of an error (status not 200). The second is a new event htmx:cancelPolling and the third is a new response header HX-Cancel-Polling.

document.addEventListener('htmx:cancelPolling', function(evt) {
    if (evt.detail.xhr.status !== 200) {
      evt.preventDefault();
    }
});

Corresponding issue: #2489

Testing

Unit tests added.

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

src/htmx.js Outdated
cancelPolling(elt)
}
if (hasPolling(elt) &&
(xhr.status === 286 || // TODO deprecated, removed in 2.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if this magic status code can be removed from this PR or if we should keep it?

Copy link
Contributor Author

@xhaggi xhaggi May 29, 2024

Choose a reason for hiding this comment

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

Instead, we could add an HX-Cancel-Polling response header, couldn't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now added the response header.

Copy link
Contributor

Choose a reason for hiding this comment

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

286 will be kept in 2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1cg removed the comment and force-push both this PR and the v1 backport PR. Anything else?

@Telroshan Telroshan added the enhancement New feature or request label May 30, 2024
@xhaggi xhaggi force-pushed the cancel-polling-on-error branch from 913d42d to ce0f16a Compare May 30, 2024 08:36
@xhaggi xhaggi force-pushed the cancel-polling-on-error branch from ce0f16a to 9024efa Compare July 9, 2024 06:54
@xhaggi xhaggi requested a review from 1cg July 9, 2024 06:57
@tacman
Copy link

tacman commented Aug 8, 2024

this looks great! I'd love to combine it with a way to automatically retry "server busy" until a better status code (200, 404, 50x) is returned. In particular, the server-busy seems made for this type of situation, it'd be great if it could be the default.

@MichaelWest22
Copy link
Collaborator

One note is that when you call cancelPolling() it sets cancelled = true on internal data which makes the next polling timer run do nothing and not schedule a new timer but it does not clear the timeout number. This means that your hasPolling() function will keep returning true even after the polling is canceled leading to your cancel checking code running again every time. Not that this is a real issue as it could just cancel it twice doing nothing bad and it would be rare for it to run it again anyway once the polling is canceled unless you have another trigger. So the only downside is that the hasPolling() function is not 100% accurate and could cause confusion if it is relied on in the future. So this could be solved by either adding

return getInternalData(elt).timeout > -1 && !getInternalData(elt).cancelled

so that it returns the true state even though it doesn't really matter to much for its current use case. Maybe could get cancelPolling to remove the timer fully but that may need more testing. Other option would be to remove the hasPolling() utility function and just using:

   // If polling timeout set check for polling cancel conditions
    if (getInternalData(elt).timeout > -1 &&
        (xhr.status === 286 ||
        (hasHeader(xhr, /HX-Cancel-Polling:/i) && xhr.getResponseHeader('HX-Cancel-Polling') === 'true') ||

To simplify the code and avoid the utility function confusion. but I think it would need a comment in the code so its clear timeout means polling

@xhaggi xhaggi force-pushed the cancel-polling-on-error branch 3 times, most recently from 6c19e07 to b3ce751 Compare August 15, 2024 18:55
@xhaggi
Copy link
Contributor Author

xhaggi commented Aug 15, 2024

@MichaelWest22 thanks for your review 👍 The cancelPolling function should now clear the timeout if any and set the internalData.timeout = -1.

@xhaggi xhaggi force-pushed the cancel-polling-on-error branch from b3ce751 to 450e9f6 Compare August 15, 2024 19:09
@MichaelWest22
Copy link
Collaborator

if we cancel the trigger like you have done then the cancelled property becomes redundant and would not be needed anymore and could be removed from line 700, 2245, 2265 as this property will never be accessed.

@xhaggi xhaggi force-pushed the cancel-polling-on-error branch from 450e9f6 to e17a377 Compare August 16, 2024 05:35
@xhaggi xhaggi force-pushed the cancel-polling-on-error branch from e17a377 to 77056cc Compare August 16, 2024 05:38
@xhaggi
Copy link
Contributor Author

xhaggi commented Aug 16, 2024

@MichaelWest22 You're right, I overlooked that. Thank you again 👍.

@MichaelWest22
Copy link
Collaborator

I like the idea of having an event to trigger the end of polling and I think it fits in really well with the event driven nature of htmx.

But I don't know if the config item option is ideal as it is likely to cause confusion and bugs for future users as with this simple option set the polling will stop on any error even transient network errors the developer does not expect it to stop on. Using the 286 response code or a custom htmx:cancelPolling event handler allows the developer to easily configure what conditions the polling will cancel on and adjust the cancel condition as edge cases are discovered.

Also the additional response header cancel option adds more complexity to the list of headers to learn and if you have the ability to set that header to cancel polling you can just as easily set the 286 status code probably?

dev...MichaelWest22:htmx:pull-for-event

For example here is my quick version with just the cancel event implemented and also another cancel option I think fits well with htmx usage where you can set a max poll duration with hx-trigger="every 1s for 60m".

@gerbil
Copy link

gerbil commented Oct 4, 2024

Any progress here?

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

Successfully merging this pull request may close these issues.

6 participants