High Wire: Fix appendJs when invoked from inside a toJsCmd def #1711
Conversation
This will change the way you have to invoke jsToAppend to require parens, but allows for jsToAppend to clear the appendable JS after having assembled it to be returned. This ensures Lift’s internals won’t double-evaluate jsToAppend in weird ways. It may be worth making this private[http] and exposing a public version that takes no parameters and sets clearAfterReading to false, we’ll see.
Places that return JS via a JsCommands toResponse call now add jsToAppend after evaluating all JS commands passed to the JsCommands class. What this allows is for JsCmd classes that implement toJsCmd as a def that in turn appends JS via S.appendJs, to still send down the appended JS. Before, because the toJsCmd calls were done after jsToAppend was evaluated, that JS would be lost or sent down in a future AJAX response. Additionally, JsCommands and LiftMerge now clear the jsToAppend after reading it, to ensure that if two JsCommands are used, you won’t get duplicate appended JS.
|
I have a rather large app I can test with. We use JsCmds in all possible ways, I think. |
|
I did some (quick) testing of our app now and it seems all usages of I found an inssue where |
|
It seems the contextPath-issue only manifests it self when the JS-content is generated in another thread (job on thread-pool), not having access to Req of Session. Forget about this for now... |
|
Sweet! I also need to tweak up the Scaladocs (or add them in the case of |
|
Ping:-) Any progress on merging this? |
|
Haha I was just looking at it today. Want to do that scaladoc update and then merge… Tomorrow night, probably, if that doesn't seem too long. |
|
Rock on! |
This lets us be explicit that calling it clears S.jsToAppend.
|
Looks like both failures were the typical failures with futures… @farmdawgnation you interested in |
?
|
👀 |
|
@farmdawgnation Thinkin' 'bout merging this tomorrow come hell or high water; lemme know if you spotted anything problematic :) |
|
I didn't. I intended to build a sample project with it and try to break it but got distracted by other things. :( |
|
No worries. Let's ship it since Andreas confirmed it works on their existing codebase. |
High Wire: Fix appendJs when invoked from inside a toJsCmd def appendJs should append JavaScript to be sent down with the current request. However, if appendJs is called in a toJsCmd method, and that method is defined as a def, not a val, then toJsCmd will be called after the jsToAppend is read. This means that this appended JS won't be sent down. One place where this manifests is when a JsCmd's toJsCmd method sends down a rendered snippet that contains a Wiring component (because Wiring uses appendJs). In this case, the appendJs that Wiring does is evaluated during the toJsCmd invocation, which means the JsCmd that is sent down misses it. When we send down JS in bulk for an AJAX or comet response, we wrap it in a JsCommands, which is a collector of JsCmd. We then call its toResponse to get a JavaScriptResponse. Before, we passed the jsToAppend to JsCommands, and then let JsCommands run toJsCmd on everyone while constructing the response. In this PR, let the toResponse method call toJsCmd on all the commands that are explicitly being sent down, and then we read jsToAppend and concatenate that on the end of the other commands inside toResponse. In the process, we remove external invocations of jsToAppend. So as to avoid accidental duplicate send-downs of jsToAppend, we give jsToAppend a clearAfterReading boolean that, when set to true (it defaults to false), will clear the list of JsCmds that need to be appended. JsCommands and the jsToAppend reading in CometActor, which sends down additional JS as a partial update, both set it to true. Amongst other things, this ensures the two won't both try to send down the same JS.
|
Yahoo! |
This fixes #1708 specifically.
appendJsshould append JavaScript to be sent down with thecurrent request. However, if
appendJsis called in atoJsCmdmethod, and that method is defined as a
def, not aval, thentoJsCmdwill be called after thejsToAppendis read. Thismeans that this appended JS won't be sent down.
One place where this manifests is when a
JsCmd'stoJsCmdmethod sends down a rendered snippet that contains a Wiring
component (because Wiring uses
appendJs). In this case, theappendJsthat Wiring does is evaluated during thetoJsCmdinvocation, which means the JsCmd that is sent down misses it.
When we send down JS in bulk for an AJAX or comet response,
we wrap it in a
JsCommands, which is a collector ofJsCmd.We then call its
toResponseto get aJavaScriptResponse.Before, we passed the
jsToAppendtoJsCommands, and thenlet
JsCommandsruntoJsCmdon everyone while constructingthe response. In this PR, let the
toResponsemethod calltoJsCmdon all the commands that are explicitly being sent down, and then
we read
jsToAppendand concatenate that on the end of the othercommands inside
toResponse. In the process, we remove externalinvocations of
jsToAppend.So as to avoid accidental duplicate send-downs of
jsToAppend, wegive
jsToAppendaclearAfterReadingboolean that, when set totrue(it defaults tofalse), will clear the list ofJsCmds that needto be appended.
JsCommandsand thejsToAppendreading inCometActor, which sends down additional JS as a partial update,both set it to
true. Amongst other things, this ensures the two won'tboth try to send down the same JS.
Still pending here: I (or someone else) need to test some of the
scenarios of
appendJs, like calling it from inside a comet, froma regular AJAX request, and from a regular screen render.