Intercooler.processNodes is not idempotent
fsateler opened this issue · 8 comments
processNodes is not idempotent, leading to duplicate requests if it is called multiple times on the same element:
Here's a jsfiddle documenting the behavior:
https://jsfiddle.net/cnjzt72b/1/
This is problematic when attempting to integrate Intercooler with some ajax-loading framework (like turbolinks), as some nodes might be processed twice leading to duplicate requests/actions.
For now I've been guarding processNodes with if (!$(this).data('elementAdded.ic')), but I think IC should do this on its own.
Interestingly, the duplication appears to only happen once: running processNodes several times in the jsfiddle still only leads to one duplicate, which suggest IC is trying to be idempotent but has an edge case that fails.
Fascinating. If there's an edge case not being handled as intended, that's certainly worth looking at for a fix. However, I'm curious to hear more about why you're using both Turbolinks and Intercooler in the same app. :) I'm not familiar with it, but a quick look at Turbolinks, and its "sister framework" Stimulus, shows they're quite similar in concept to Intercooler. That's great! It's nice to have confirmation of a good idea. But why not do the main page navigation with Intercooler itself and therefore not have to call processNodes() at all?
Mostly because laziness 😉
Turbolinks comes by default with rails apps. We needed some small interactions (ajax posts and whatnot), and intercooler is very nice because of its simple "just spit out html and it will be inserted into the page".
Stimulus JS was not available at the time (or at least I did not know about it), and it seems to be a framework for integrating javascript into your page, rather than extending the html language. That is not what I want.
As to why I don't replace turbolinks with Intercooler, that's because Turbolinks has some very nice features that I would have to implement with IC: progress loading and caching.
Shows how out of touch I am with the Rails ecosystem. :) Thanks for explaining the benefits you see in both Intercooler and Turbolinks. And of course Intercooler should play nice with other frameworks in general.
Looking at the code of processNodes(), I see that many aspects of it are already idempotent, so we can focus on the specific problem that you're seeing. In your jsfiddle, the problem is that when the button is clicked, two requests are fired. That suggests that Intercooler is attaching two event handlers to the button when processNodes() is called again. When processNodes() is called additional times, it appears that the number of requests that get fired is limited to two, which could either mean that only two event handlers are getting attached or that the third and later requests are somehow being suppressed.
Sure enough, handleTriggerOn() does add an event handler without checking whether one has already been added. This would seem to confirm that multiple event handlers are getting added, which means that requests beyond the second one are getting suppressed.
Digging a little deeper, we can see that fireICRequest() is checking whether there is currently a request in flight on the given element before initiating another one. If it is, then the next request is queued up to run after the request that is in flight. However, this "queue" is only ever a single additional request! So if we call processNodes() five times, then five event handlers are created and triggered, but only the first and the last actually have their requests executed.
That means there are actually two independent bugs in play here:
- processNodes() should ideally not create an event handler on an element if it has already done so.
- fireICRequest() should be able to queue up more than one request on an element.
Hey Guys,
OK, I've begun work on intercooler 2.0, renamed kutty:
The equivalent code there is idempotent, but we still only allow one request at a time per element.
Hmmm, I understand that HTMX has this implemented, but for those of us that's going to be using Intercooler for a while still... how hard would it be to make the processNodes method idempotent? This is biting us at the moment, due to an ajax grid that we're using 😬
Ha! I was able to hack our particular problem by specifying the "once" ic-trigger-on modifier. I reckon it should be good enough for our current use case at least.