Comet, Comet on the Wall: Fix comets sent down via AJAX from within a comet #1646
Conversation
We do this because jsToAppend is referenced in a few different places, and it’s better to reuse it than to try and add commandForComets to all of those places (especially since some of them are inside CometActors, for example).
When dealing with an AJAX request in a comet context, we were partial-updating jsToAppend at the beginning of the request, but any updates that might have been made to it during the request processing were lost. We now send the partial update message after the request functions have run, so that any JS they’ve appended also gets sent down to the client.
If a comet was sent down from within a comet context, we would force a comet request reset. On the client, this would schedule a new comet request and attempt to abort the old one; however, there is no way to abort the old one, since its success handler is what is kicking off this request reset. The end result is that the request’s success handler finished running, which would also schedule a new comet request, and we’d get two comet requests running in parallel and walking all over each other. When generating the comet commands, we now check to see if we’re executing in a comet context, and if we are we ask the client not to reset the comet cycle, knowing that the response we’re currently sending will immediately kick off a new request once it’s done executing.
Shadowfiend
force-pushed
the
comet-comet-on-the-wall
branch
from
b33817e
to
48e1e05
Compare
We now pull jsToAppend handling out of the message handler for comet-context AJAX request processing all the way to the wrapper around handling all messages. This ensures that things like partial updates will properly render their JS to append. It also seems to incidentally fix S.jsToAppend within comet rendering, which seemed to trigger the JS twice instead of just once before this.
| S.jsToAppend() match { | ||
| case Nil => | ||
| case js => partialUpdate(js) | ||
| } |
There was a problem hiding this comment.
My only concern here is this, which was originally added to deal with some race conditions with wiring, I believe. I don't think what we're doing now will be significantly different, since the fact that it's a partialUpdate in both cases means it will be processed as a subsequent message to the comet in both cases, but it's worth pointing out in case anyone disagrees.
|
Thoughts on this? |
|
I haven't gotten a chance to test this but is this supposed to fix the issue where using wiring from within a lazy-loaded snippet (added using AJAX) doesn't work? |
|
It may fix that… But it may not. This fixes issues with sending comets down from within AJAX requests that are bound within other comets, amongst other things. It would also fix any other issues with In general, it fixes most uses of The original issue that caused me to investigate and fix is on the mailing list. |
|
Worth mentioning, https://github.com/gvdm/lift-multi-chat can be used to see the intended fix. |
| case Nil => Empty | ||
| case x :: Nil => Full(x) | ||
| case xs => Full(xs.reduceLeft(_ & _)) | ||
| protected implicit def nsToNsFuncToRenderOut(f: NodeSeq => NodeSeq) = { |
There was a problem hiding this comment.
Any chance of us renaming this to nodeSeqFuncToRenderOut?
There was a problem hiding this comment.
And changing f to something else?
There was a problem hiding this comment.
Maybe as part of some future cleanup pass. Though tbh nodeSeqFunc isn't quite right (this is specifically NodeSeq to NodeSeq) and the difference is small enough, especially when tucked into an implicit which will never be explicitly invoked, that it's not really a big deal IMO.
|
K, fixed the one style thing I wanted to fix before this went in. Going to send it in tomorrow (hopefully heh) if nothing else comes up. |
|
Lighting this green. |
Comet, Comet on the Wall: Fix comets sent down via AJAX from within a comet This fixes a few issues: - We centralize comet command generation into S.jsToAppend. - We fix when we look up S.jsToAppend during AJAX processing in comet contexts so we don't miss anything that might be added during the actual AJAX function execution. - We make sure not to trigger a second comet request when setting up new comets client-side from within a comet response.
This fixes a few issues:
S.jsToAppend.S.jsToAppendduring AJAX processing in comet contexts so we don't miss anything that might be added during the actual AJAX function execution.Fixes issue #1645 .