[wp-trac] [WordPress Trac] #23216: Create "WP Heartbeat" API

[WordPress Trac]

#23216: Create "WP Heartbeat" API
--------------------------------------+-----------------------
 Reporter:  azaozz                    |       Owner:
     Type:  task (blessed)            |      Status:  reopened
 Priority:  normal                    |   Milestone:  3.6
Component:  Administration            |     Version:
 Severity:  normal                    |  Resolution:
 Keywords:  autosave-redo needs-docs  |
--------------------------------------+-----------------------

Comment (by carldanley):

 Replying to [comment:96 azaozz]:
 > I would have really appreciated another pair of eyes on this couple of
 months ago, when heartbeat was in development. Restructuring/rewriting
 parts of it in RC seems too late.

 I do apologize for not being available earlier. This was my first
 available time, and Nacin suggested that I look at this ticket. I hope you
 will find my revised contribution tomorrow morning to be helpful even at
 this late stage. I will be taking all of your comments into consideration
 as I prepare it. As the current implementation of this feature is still
 "experimental", I figured a second set of eyes wouldn't hurt. I know it's
 late in RC but I think there was room to make improvements in the existing
 iteration and prevent any security flaws without affecting the integrity
 of this feature. I fixed a few funky things and added "to do" notes for
 areas that need help in future iterations of WP.

 > Currently the API is in "experimental" stage. The usefulness of the
 initial idea was severely reduced by concerns it will overwhelm shared
 hosting accounts because it connects every 15 seconds while a user is
 logged in. It is structured as a (pseudo) class so it can have private and
 public vars and methods, however that class is intended to be initialized
 only once and is not intended to be extendable.

 I agree that iteration and experimentation are a great way to push
 forward. Hopefully my contributions now will help all of us in later
 iterations. The code in the patch I supplied is also structured as a
 module with public and private variables & methods as well; this was
 intended so the patch matched the modular approach that is currently in
 place.

 > In that terms the _Cashe and _Settings objects in 23216-heartbeat-
 cleanup.diff​ ate pointless. If the concerns that connecting every 15
 seconds is too much to handle for an average host are proven to be true,
 heartbeat would probably be reduced to couple of helper functions. If
 these concerns are proven wrong, heartbeat can be made into a "real" JS
 class (or couple of classes) that can be extended, and maybe support
 several instances running at the same time, etc.

 _Cache and _Settings are a much more organized approach to clearly
 defining variables into an appropriate namespace. _Cache and _Setting were
 intended only to make the current code easier for other developers to
 understand so that they can contribute in later iterations. I think there
 was a lot of confusion in the 17 line long variable declaration with no
 prefix. With _Cache and _Settings we now know what the purpose of each
 privately scoped module variable is and where it belongs. When reading
 through the current iteration it was really difficult to discern the
 difference between global module variables and function variables.

 > Regarding coding standards:
 > - The use of curly brackets on `if` is optional in the WordPress coding
 standard. This is mostly a readability thing as the minimizer adds and
 removes brackets where needed, so the production (minimized) code is
 unchanged.

 Even though it gets minimized, it's still important that we maintain
 proper standards going forward. This style is largely based off of both:
 http://make.wordpress.org/core/handbook/coding-
 standards/javascript/#ifelse and http://contribute.jquery.org/style-
 guide/js/#spacing and is something that is common advice throughout most
 modern JS frameworks.

 > - A single space is required after `if`, `for`, `while`, etc.
 > - The `else` and `else if` should be on the same line as the preceding
 curly bracket.

 Thank you for those notes. I will patch accordingly.

 > - You mention this couple of times but I don't see any missing/added
 semicolons in 23216-heartbeat-cleanup.diff :)

 Mostly because I removed the use of NFE's which lead to anonymous
 debugging. Here are a few examples of missing semi-colons in the current
 iteration of `heartbeat.js`:
 * L208 - unnecessary semi-colon after function declaration
 * L347, 423, 450, 461 - missing semi-colon after named function expression
 variable declaration

 > Using `switch () { }` instead of a long list of `if () { } else if () {
 } else if () { }`, seems more readable. But this is a personal preference.

 I agreed that the transition from `switch` to `if...else if...else if...`
 is not important. However, after researching a lot of different use-cases
 for switches, it seems like I'm sold on avoiding them when possible due to
 memory leaks in older browsers. You can thank Douglas Crockford for my
 dislike of `switch` statements =P

 > Wrapping a DOM node in jQuery is very fast (it bypasses all the css
 selectors logic, etc.). Not sure caching of `$(document)` and `$(window)`
 makes any significant difference but doesn't hurt. I suppose `$(document)`
 is as readable as `$document` for most people, `_Cache.$document` might
 not be.

 I tried to find the jsperf test that demonstrates the speed benefits of
 caching these (or any) jQuery selectors and I couldn't find it off the bat
 BUT this test will do for now: http://jsperf.com/jquery-window-query-
 caching2 . I can guarantee you can run it in any browser and the cached
 value will be *much* higher. That specific test isn't too saturated but
 I'm positive of the benefits in this simple caching routine and that this
 will speed up the module just a tad.

 >
 > Having a lot of inline comments is nice, explaining too much may feel a
 bit strange though:
 > {{{
 > // set the value of hasFocus to false
 >  _Settings.hasFocus = false;
 > }}}

 That comment was pretty goofy of me. Didn't do it on purpose =P. I usually
 try to avoid comments like that as they are quite redundant. I will be
 happy to remove redundant comments like this in the morning.

 >
 > Like the function names changes, not sure all functions in the private
 scope have to start with an underscore. Don't see this anywhere else in
 WordPress. Also not sure it needs all the new private functions. Maybe
 they make heartbeat a little bit slower after the patch.

 Prefixing with an underscore is a preference but I think it's a good one
 as it clearly denotes the use of private methods in this module. I can't
 speak for the rest of WordPress but I know that either way clear code is
 always better. As for performance, I highly doubt that there are any
 performance hits.

--
Ticket URL: <http://core.trac.wordpress.org/ticket/23216#comment:97>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software