[Vortex] JPush dispatcher

Review Request #700 — Created July 15, 2020 and submitted

smillernl
Lunr
feature/vortex/jpush_dispatcher
HKG-1262
699
705
lunr
Vortex: JPush dispatcher


  • 0
  • 0
  • 3
  • 0
  • 3
Description From Last Updated
pprkut
  1. 
      
  2. src/Lunr/Vortex/JPush/JPushDispatcher.php (Diff revision 1)
     
     
     
     
     
     

    Is that confirmed from their documentation?

    1. Yes, should I add a link?

    2. No, just checking :)

  3. I would do this like for FCM, where registration IDs are set directly in the dispatcher.

    1. I think that messes with the seperation of concerns (payload manages the payload) and I'd rather make the FCM one add it in the payload too.

    2. That's valid, but on the flip-side, adding endpoint setting to the payload adds ambiguity (from an API user's perspective) on the order in which they have to be set, since both dispatcher and payload need them.

      Right now the separation is pretty clear. Payloads don't know the endpoints, for none of the supported platforms, not even email. The dispatcher fetches the endpoint independent payload, adds the endpoints, and sends it on its way.

    3. For reference:
      In slack I suggested using a static $payload::set_registration_ids($payload->get_payload(), $endpoints); to avoid both having payload building logic in the dispatcher and to avoid the payload accidentally becoming stateful.

    4. So, I gave this a good long thought (sorry). TL;DR: endpoints should not be set on the payload object.

      Here's why:

      • The payload classes don't abstract the payload, but collect payload data. That's a subtle difference, but it's precisely why for WNS or PAP, for example, the actual payload is constructed in the dispatcher class. The payload class is supposed to hold all payload elements that are static, that can be set once, with the dispatcher supplementing all variable information that needs to be set per push. Endpoints fall into the latter category.
      • The payload class having a public method to set endpoints results in a confusing interface. From a developer's perspective, both payload and dispatcher would take a list of endpoints, both would look sensible, but only one would work. Endpoints set by the developer on the payload would be overwritten by the dispatcher, resulting in very unexpected behavior. And further confusion when someone tries to figure out how to specify the endpoints for multiple batches in the payload (which might result in someone instantiating multiple payloads just to be able to specify different endpoints, which are then not even used). In a best case scenario this results in wasted resources, and in the worst, very hard to debug bugs.

      I understand the argument to fully abstract the payloads in the payload classes, but that's a larger construction site that would need to be done across the board, with careful thought on how it would affect the interface.

  4. 
      
smillernl
smillernl
pprkut
  1. 
      
  2. src/Lunr/Vortex/JPush/JPushDispatcher.php (Diff revisions 2 - 3)
     
     

    json_encode($tmp_payload)

  3. 
      
smillernl
Review request changed

Status: Closed (submitted)

Loading...