search

MihaelIsaev / FCM

⚡️ PushNotifications through FireBase for Vapor 3 and 4.
MIT License
120 stars 33 forks source link

Fix apple payload datamodel access #14

Closed FredericRuaudel closed 4 years ago

FredericRuaudel commented 4 years ago

In order to test my model properties, I need to access the properties values of the APNs data models.

As they are already public in the Android side, I imagine this is not a deal breaker to homogenize it on the Apple side?

I just have a few doubts on the final result:

  1. should we make them accessible only in readonly mode with a private(set) (I didn't add it since Android is fully public)
  2. To make this work, I had to make FCMApnsAlertOrString public to make Swift compiler happy in FCMApnsApsObject (aka: can't set property public because type used in not public yadiyada)
  3. Not sure on the naming of the FCMApnsAlertOrString helpers to access the enum associated values: alertPayload/alertMessage or asPayload/asMessage or asPayload/asString

at call site it looks like this right now:

iosConfig?.payload.aps.alert?.alertPayload?.titleLocArgs

so maybe it would look better like this:

Config?.payload.aps.alert?.asPayload?.titleLocArgs

🤔

What do you think?

Thanks in advance!

MihaelIsaev commented 4 years ago

@FredericRuaudel thank you for the pull request! ❤️

  1. Maybe it'd look better, yeah 🙂 but we definitely should keep old way too just by marking it deprecated to save backward compatibility
FredericRuaudel commented 4 years ago

Maybe it'd look better, yeah 🙂 but we definitely should keep old way too just by marking it deprecated to save backward compatibility

I'm not sure we understand each other here, I've added those computed properties so I could have change the name right now and no need for deprecated marker. Or I didn't get what you said? 🤔

MihaelIsaev commented 4 years ago

Maybe it'd look better, yeah 🙂 but we definitely should keep old way too just by marking it deprecated to save backward compatibility

I'm not sure we understand each other here, I've added those computed properties so I could have change the name right now and no need for deprecated marker. Or I didn't get what you said? 🤔

Yeah, my bad, sorry I was in a hurry 🙂 no questions! all good! 🙂