Fixes T5344. Essentially, we only make the AJAX request to /notification/individual/ if we are the leader tab (i.e. only one tab will make this request). Once a response has been received from the server (containing the contents of the notification), we broadcast the message contents back to all other tabs for rendering.
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T5344: Multiplex AJAX calls from Aphlict clients through the JX.Leader
- Commits
- Restricted Diffusion Commit
rP4135752490b8: Multiplex AJAX calls
Opened two tabs on /notification/status/ and clicked "Send Test Notification".
Before
[Tue, 13 Jan 2015 20:10:37 +1100] 17033 phabricator 10.0.0.1 josh PhabricatorNotificationIndividualController - /notification/individual/-200 236036 [Tue, 13 Jan 2015 20:10:37 +1100] 17657 phabricator 10.0.0.1 josh PhabricatorNotificationIndividualController - /notification/individual/-200 24130
After
[Tue, 13 Jan 2015 20:11:15 +1100] 17657 phabricator 10.0.0.1 josh PhabricatorNotificationIndividualController - /notification/individual/-200 180217
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
This looks good to me, but some structural feedback inline. Does that stuff seem reasonable? The handler/renderer split feels "application-ey" to me, although I think my objection is pretty subjective.
When I looked through this at first, I was thinking we should short-circuit the behavior when we receive a message from the server (i.e., not broadcast it), but the more I think about it the more I like this approach. We'll send every tab a message and currently they'll all ignore it, but localSocket seems really fast, and this is a lot more general and cleaner than special-casing the websocket messages.
webroot/rsrc/js/application/aphlict/Aphlict.js | ||
---|---|---|
119–123 | This feels kind of too-coupled to me. In particular, there's a lack of symmetry between the handler (which just calls JX.invoke) and the renderer (which has a ton of application logic). I think it would be cleaner to make this a default: case, call the handler, and pass it the message and the is_leader flag. Then the handler would look like: switch (message.type) { case 'aphlict.server': JX.Stratcom.invoke('aphlict-receive-message'); // maybe rename to aphlict-server-message? break; case 'notification.individual': JX.Stratcom.invoke('aphlict-notification-message'); break; } Basically, handle protocol logic here but delegate all application logic to the handler callback. | |
webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js | ||
26–28 | With the adjustments above, we could also make is_leader available in these listeners, which would simplify them a bit (they wouldn't need to use a callback here). | |
70 | This would become JX.Stratcom.listen('aphlict-notification-message', ...) or whatever. |
Thanks, useful feedback. My JavaScript isn't great and I've never really used Javelin before.
Woah, this ended up really clean after separating all the minor stuff into D11398
webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js | ||
---|---|---|
26–29 | Not a big deal, but it would be vaguely nice to pass is_leader through here eventually to avoid this callback. I don't think it's worth including in this diff, but maybe the next time we touch this stuff. |
One minor issue with this approach (although theoretical) is that the AJAX request from the leader tab will fail. Previously, if one of the AJAX requests to /notification/individual/ failed then all other tabs would still receive and render the notification (assuming that they were able to make a successful request to /notification/individual/). I guess that this type of failure should be handled by JX.Router, but I'm not too familiar with Javelin.