Fiery Comets: Support for AJAX responses with comets (shifted to master) #1585

Shadowfiend · Jun 20, 2014

This is #1565 reopened with respect to master.

To support AJAX responses that send down comets, this PR actually
refactors a few aspects of comet creation:

LiftSession now has a new internal (private) API for finding or creating
a comet. There are two variations of findOrCreateComet, one that takes
a type parameter (findOrCreateComet[MyCometType](name, html, attributes))
and one that takes a string comet type (findOrCreateComet("MyCometType", ...))
The type parameter version uses manifests to allow direct instantiation of a
comet without reflection overhead and without requiring the comet to be in the
regular comet package if not wanted.
LiftSession's comet-related code has been significantly refactored to
eliminate duplication and make things a little easier to follow. Notably, there
are still some anonymous subclasses of CometActor created for round-trips
and streaming promises; those will be refactored to independent subclasses
at some point, but I didn't do it in this PR.
Comet ID info (type + name) now has a (private) case class CometId to represent
it in LiftSession.
The CometCreationInfo case class, which was in the Templates.scala file, is
now in the CometActor.scala file, and is used pervasively to pass comet creation
info around (before, we were using separate parameters in many places). This is
still mostly an internal class, however.
Publicly, S has a new API for finding or creating a comet, S.findOrCreateComet.
It works much like the LiftSession API, taking either a type parameter or a string
that is a comet class's name. Both of the overloads also have a
receiveUpdatesOnPage parameter that is defaulted to false. When set to true,
it will automatically append the created CometActor to the page or AJAX request
so that it will receive updates.
S also has a method for adding a comet to the currently rendering page or page
that invoked the current AJAX callback, S.addComet. It takes a LiftCometActor,
which you can get from S.findOrCreateComet if you don't have a direct handle
to it already.
The internal client-side JavaScript API now supports aborting the current comet
request, mostly in order to support restarting the current comet cycle. This is used
when a new comet is received from the server to ensure the comet starts receiving
updates immediately.
The comet snippet has been updated and simplified to work with these changes.
It also now has a containerForCometActor method that is publicly available and
lets you get the HTML container for any given comet actor. This lets you do your
own work to create a comet container without directly invoking the comet snippet.

farmdawgnation · Jun 28, 2014

It's on my to-do list to do a pass on this PR this weekend. I will (hopefully) actually get to that pass. Want to test this by building a sample project and seeing if I can break it.

Shadowfiend · Jul 2, 2014

Any news? :)

farmdawgnation · Jul 2, 2014

Still on my list.

farmdawgnation · Jul 4, 2014

Haven't looked at the code quite yet, but I was able to do a successful test of sending down a comet via AJAX!

You can find the sample project I used here: https://github.com/farmdawgnation/fiery-comets-test

farmdawgnation · Jul 4, 2014

farmdawgnation reviewed Jul 4, 2014

View changes

farmdawgnation · Jul 4, 2014

web/webkit/src/main/scala/net/liftweb/builtin/snippet/Comet.scala

-   *   &lt;lift:comet type=&lt;Your comet class> name=&lt;Optional, the name of this comet instance>>{xhtml}</lift:comet>
-   * </pre>
+   * {{{
+   *   <lift:comet type="MyCometClass"> name="Optional, the name of this comet instance">{xhtml}</lift:comet>


Can we change this to use the data-lift syntax while we're at it?

farmdawgnation · Jul 4, 2014

farmdawgnation reviewed Jul 4, 2014

View changes

farmdawgnation · Jul 4, 2014

web/webkit/src/main/scala/net/liftweb/http/CometActor.scala

+                                   cometName: Box[String],
+                                   cometHtml: NodeSeq,
+                                   cometAttributes: Map[String, String],
+                                   session: LiftSession)


Would we benefit from providing default attributes for any of these?

This is used internally at the moment, but has been public API since it was introduced. I'm not messing with it for now, we'll see in the future. Interactions with it should generally go through the S methods anyway.

farmdawgnation · Jul 4, 2014

farmdawgnation reviewed Jul 4, 2014

View changes

farmdawgnation · Jul 4, 2014

web/webkit/src/main/scala/net/liftweb/http/CometActor.scala

-                               name: Box[String],
-                               defaultHtml: NodeSeq,
-                               attributes: Map[String, String]): Unit
+  protected def initCometActor(creationInfo: CometCreationInfo): Unit

  def theType: Box[String]


This should be changed to a String since no-type Comets aren't supported anymore right?

I believe this is still here because I didn't get a chance to fully extricate it. Since it's rarely used except internally in comet stuff, I'd rather put off completing this removal until a later PR.

farmdawgnation · Jul 4, 2014

farmdawgnation reviewed Jul 4, 2014

View changes

farmdawgnation · Jul 4, 2014

web/webkit/src/main/scala/net/liftweb/http/CometActor.scala

        case (Full(xml), _, true) => LiftRules.jsArtifacts.setHtml(id + "_outer", (
-          spanFunc(0, Helpers.stripHead(xml)) ++ fixedXhtml.openOr(Text(""))))
+          spanFunc(Helpers.stripHead(xml)) ++ fixedXhtml.openOr(Text(""))))


What were these ints used for anyway?

We were using a lift:when attribute to handle some aspects of client-side comet registration at some point in the past (in particular, I believe to indicate the current version of the comet at render time so you could get any messages after it when the browser fired up the comet connection). It's no longer used, however.

farmdawgnation · Jul 4, 2014

farmdawgnation reviewed Jul 4, 2014

View changes

farmdawgnation · Jul 4, 2014

web/webkit/src/main/scala/net/liftweb/http/LiftServlet.scala

+        true
+      ).cmd
+    } else {
+      js.JsCmds.Noop


Can we do some package imports so we don't have to mention these package names all throughout this function?

I intentionally didn't do that because it feels like this code block is doing too much JS work right now. I didn't figure out how to fix it when I first opened the PR, but I'd like for it to stand out as a sign that we should probably be doing this a different way. Hopefully I'll get to revisit it at some point, since I think there's still some good comet work to be done.

farmdawgnation · Jul 4, 2014

farmdawgnation reviewed Jul 4, 2014

View changes

farmdawgnation · Jul 4, 2014

web/webkit/src/main/scala/net/liftweb/http/LiftSession.scala

+
+    attemptedComet match {
+      case fail @ Failure(_, Full(e: java.lang.NoSuchMethodException), _) => 
+        val message = s"Failed to find appropriate comet constructor for ${cometClass.getCanonicalName}. Tried no arguments and (LiftSession, Box[String], NodeSeq, Map[String,String])"


Do you think the added information about which variants of constructors were tried will be valuable to the end user? Seems like something that could quickly become a lie (constructors change, message doesn't). Would it inform their action?

It is a huge hint on what actually went wrong, and I think more detail is always a better move in cases where we're trying to implicitly invoke something without the user telling us that's what we should be invoking. Moreover, I think essentially nobody will run into this particular issue, as a no-arg constructor is the way most people use comet actors. In the future, it might be worth exploring how to unify the message with the code to make it harder to let those two get unsynced.

Also fingers crossed on code review catching such a change, of course ;)

Ahhhhhhhh ok, after chatting with you I now understand how this error is generated. I didn't previously... read like something internal that the user couldn't fix. Perhaps this message could read...

Couldn't find valid comet constructor for ${cometClass.getCanonicalName}. Comets should have a no argument constructor or one that takes the following arguments: (LiftSession, Box[String], NodeSeq, Map[String,String]).

How does that sound?

Shadowfiend · Jul 6, 2014

A broad comment before I respond on more specific stuff: my general policy when working on Lift is “clean up/refactor as much as I'm motivated to in a given pass”. Other folks are welcome to do additional cleanup and refactoring, but don't be surprised if I turn down the option of doing additional work on a given corner that's unrelated to the feature work.

farmdawgnation · Jul 22, 2014

Hey Antonio, if you can give me your thoughts on this line comment - that's the only thing I want an answer on before merging this. :)

farmdawgnation · Jul 24, 2014

Shadowfiend changed the title ~~Fiery Comets: Support for AJAX responses with comets~~ Fiery Comets: Support for AJAX responses with comets (shifted to master) Jun 21, 2014

farmdawgnation added WebKit labels Jun 28, 2014

farmdawgnation added this to the 3.0-M2 milestone Jun 28, 2014

Tweak comet snippet API documentation.
We now use div data-lift instead of lift:comet, and fix some small issues in the remaining docs.

52f3850

Update the comet constructor not found error.

b4292c6

farmdawgnation added a commit that referenced this pull request Jul 24, 2014

Merge pull request #1585 from lift/fiery-comets
Support for sending down comets via AJAX, and does various refactoring in the comet code.

19a71d7

farmdawgnation merged commit 19a71d7 into master Jul 24, 2014

farmdawgnation deleted the fiery-comets branch Jul 24, 2014

lift/framework

Join GitHub today

Fiery Comets: Support for AJAX responses with comets (shifted to master) #1585

Conversation

Shadowfiend commented Jun 20, 2014

Shadowfiend added some commits May 1, 2014

Shadowfiend changed the title Fiery Comets: Support for AJAX responses with comets Fiery Comets: Support for AJAX responses with comets (shifted to master) Jun 21, 2014

farmdawgnation added WebKit labels Jun 28, 2014

farmdawgnation added this to the 3.0-M2 milestone Jun 28, 2014

This comment has been minimized.

farmdawgnation commented Jun 28, 2014

This comment has been minimized.

Shadowfiend commented Jul 2, 2014

This comment has been minimized.

farmdawgnation commented Jul 2, 2014

This comment has been minimized.

farmdawgnation commented Jul 4, 2014

farmdawgnation reviewed Jul 4, 2014 View changes

This comment has been minimized.

This comment has been minimized.

farmdawgnation reviewed Jul 4, 2014 View changes

This comment has been minimized.

This comment has been minimized.

farmdawgnation reviewed Jul 4, 2014 View changes

This comment has been minimized.

This comment has been minimized.

farmdawgnation reviewed Jul 4, 2014 View changes

This comment has been minimized.

This comment has been minimized.

farmdawgnation reviewed Jul 4, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

farmdawgnation reviewed Jul 4, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Shadowfiend commented Jul 6, 2014

This comment has been minimized.

farmdawgnation commented Jul 22, 2014

This comment has been minimized.

farmdawgnation commented Jul 24, 2014

farmdawgnation added a commit that referenced this pull request Jul 24, 2014

farmdawgnation merged commit 19a71d7 into master Jul 24, 2014

farmdawgnation deleted the fiery-comets branch Jul 24, 2014

farmdawgnation reviewed Jul 4, 2014

View changes

farmdawgnation reviewed Jul 4, 2014

View changes

farmdawgnation reviewed Jul 4, 2014

View changes

farmdawgnation reviewed Jul 4, 2014

View changes

farmdawgnation reviewed Jul 4, 2014

View changes

farmdawgnation reviewed Jul 4, 2014

View changes

farmdawgnation merged commit `19a71d7` into master Jul 24, 2014