-
Notifications
You must be signed in to change notification settings - Fork 278
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
Lazy Susan: Implement lazy-load and future CanBind support in AJAX responses #1607
Conversation
We move the AsyncRenderComet to its own file. We also move the actual render function management into the AsyncRenderComet companion, including handling deferred function building. LazyLoad slims down a little, dealing with invoking the async render setup in AsyncRenderComet and rendering the placeholder. We also only apply a div wrapper to the default loading template, letting a custom template stand on its own, though we apply an id to it for the purposes of replacing it with the rendered contents after they are sent down.
LiftSession’s buildDeferredFunction only existed for a no-arg function. We now have a version that builds a deferred function of one argument; invoking that function with an argument will restore the session/request context and then execute the original function with the passed argument. Also simplified our restoration of context in both versions of buildDeferredFunction.
Also clean up some old commented-out package object cruft.
If we fail to find the session, we now report as much in the Failure message.
Still not as clean as I’d like, but getting there.
Now anyone can provide a CanResolveAsync implicit to deal with other future types (for example, Finagle’s Twitter Futures) and the CanBind will automatically apply.
This is way cool! I have some suggestions to make it even more useful. I often have the need for some custom execution-context and have gotten very used to LoadWrappers for providing this. I have my custom version of LazyLoadSnippet with support for LoanWrappers, with the following signature: It builds the deferredFunction like this: Where
The cool thing about it is that I can use it for programatically installing lazy-load zones where evaluation of the argument is lazy, meaning all computation is done async:
It would be cool if something like this went into Lift-3, shall I add something similar to the new LazyLoadSnippet, or will you? |
Hmm… I think I'm getting it, but I'd like a more concrete example to grasp the use case more clearly. What kinds of things do you put in your loan wrappers in these cases? It seems like that functionality would best be pushed all the way into On pertinent question that comes up is, should |
The use-case is that I often want some parts of the page to load async, that given. Sometimes these parts are part of an AJAX-response, so I have a custom version What my
I think pushing I also prefer placeholderNS (the NodeSeq to render which waiting for the data) to be an explicit argument. Let me know if the use-case still isn't clear. Lift-3 really shines with its improved AJAX/async-support, which we use quite heavily but had to hack a bit to accomplish in pre-3. |
Are the same As a side note, I'll be replying to comments/thinking about solutions, but I probably won't circle back to make code changes until the end of the week. EDIT: My thinking on |
No, the list of LWR are not the same as in |
Got it. Perhaps something like |
Excellent suggestion! |
Cool. I'll be seeing @dpp the next couple of days so I'll probably pick his brain some more on |
Okay, chatted with David a bit about this… It seems like for now your use case is still a bit unique to add to this API. To simplify it further, I'm thinking of providing an overload of the def render(xhtml: NodeSeq, renderer: (String)=>JsCmd): NodeSeq The Thoughts? |
Hm, I was hoping to avoid having to make any custom LazyLoad object. I'm quite surprised that my use-case is thought of as a bit unique. For me these things are important API wise:
AFAICS what you propose makes none of this possible. However it does make creating a custom LazyLoad object much easier. |
Sorry, yeah, I over-focused on the |
Nice! Rock on! |
This is for doing an asyncRender when the function has already been wrapped with whatever deferred wrapperisms you may want, maybe including buildDeferredFunction.
There is now an overload that takes a renderer function to which we hand the placeholder id that we generate, and which is charged in turn with doing the final rendering. It expectes the caller to wrap the passed function in whatever deferred handling may be needed. The main render method also now has an optional placeholderTemplate parameter so that when invoked from Scala it can be given a NodeSeq to use for the placeholder.
Okay, see if that works a little bit better. |
Great, I'll check it out! |
Works as expected! My custom lazy-load snippet becomes much more readable now:
I still think this should be a part of Lift so other people get this out of the box, but won't argue more about it. |
Although I just realized, your object OrigoLazyLoad {
private def around[R](lw: List[LoanWrapper])(f: => R): R = lw match {
case Nil => f
case xs => CommonLoanWrapper(xs)(f)
}
def render(xhtml: => NodeSeq, placeholderTemplate: NodeSeq, lw: List[LoanWrapper]): NodeSeq = {
LazyLoad.render((domId: String) => around(lw)(Replace(domId, xhtml)), Full(placeholderTemplate))
}
} |
That'd be great! |
Initially, the LazyLoad.render overload that took a (String)=>JsCmd explicitly didn't wrap buildDeferredFunction around the function, but after thinking about it a little more, not needing buildDeferredFunction is low-level and rare enough that we'll leave it so using that requires using AsyncRenderComet.asyncRenderDeferred instead.
Okies, there we go. |
Nice! BTW; Should't |
I've been thinking about that myself. I'm a little torn— I'm not particularly satisfied with the names as they stand, but I haven't come up with better naming. The reverse is also confusing for me. Would love to hear some impressions from other folks. |
I think it's "clean code" to name a method after what it's intention is, what it's doing. What asyncRender is doing is to process a function which it "defers" asynchronously, so IMO asyncRenderDeferred is cleaner. The name "asyncRender" implies that it's async, not that it's the caller's job to make it async... |
Heh, well the trouble is both of them handle the asynchronous part of the render. |
Yes, I know. That's why I think asyncRenderDeferred is a better name for the one handling both the asynchronicity and restoration of state. Maybe asyncRenderSnapshotted is better, the state is snapshotted and then restored when run. |
Maybe |
I don't think any of this naming-issues should hold this PR from getting merged. We should discuss on the list and open a separate PR I think. |
So is that a 👍 ? :) |
+1 for merge:-) |
Anyone want to do the honors? O:-) |
Lazy Susan: Implement lazy-load and future CanBind support in AJAX responses
Rock and roll! |
This PR really does three things:
LazyLoad
snippet and separates out theAsyncRenderComet
to its ownfile. This refactoring allows the
LazyLoad
snippet to work even if you're rendering an AJAXresponse, thanks to the fiery comets work that went in a few weeks ago.
CanResolveAsync
type class that can be used to handle any object that can beresolved to a value asynchronously in the same way. Right now, two
CanResolveAsync
implicits are provided: one for Scala
Future
and one forLAFuture
.CanBind
for any type that has aCanResolveAsync
implicit defined for it. Thisuses some of the helper functions exposed by
AsyncRenderComet
to handle asynchronouslysending down the results of resolving the object in question.
Concretely, this means four big things:
LazyLoad
now works correctly even if it's rendered during an AJAX response. This wasn'tthe case in Lift 2.x.
Future
andLAFuture
on the right-hand-side of a CSS selector transformout-of-the-box. Like @fmpwizard's original implementation and unlike my dirt-simple one, this
does not waste a thread trying to resolve the futures.
couldn't do this, I didn't check if Diego's could.
Future
s as used by Finagle) isjust a matter of providing a
CanResolveAsync
implicit. Their implementations are incrediblysimple (they are simply abstractions over
onSuccess
or whatever equivalent is provided).If you want to see this in action, check out the
lift_30
branch of mylift-future-canbind-example
repository. It mostly keeps the same code as the original example I posted a few days ago on the list, but uses Lift 3's built-inCanBind
support for futures. It also demonstrates futures and lazy loads continuing to work even when re-rendered via anidMemoize
.I've also created a
failing-lift-2
branch in that repo that has basically the same code as thelift_30
branch, but uses Lift 2.6 and the originalFutureBinds
CanBinds
. This illustrates the non-functioning AJAX lazy loading/future handling.