title-loc-args and loc-args are expected to be arrays of strings for APN

appfeel / node-pushnotifications

Push notifications for GCM, APNS, MPNS, AMZ (automatic detection from device token)

MIT License

538 stars 126 forks source link

title-loc-args and loc-args are expected to be arrays of strings for APN #150

Closed eli-zhang closed 3 years ago

eli-zhang commented 3 years ago

According to apple documentation, localization args like title-loc-args and loc-args are expected to be an array of strings, not a string of an array of strings. However, FCM documentation says the expected type is a JSON array of strings. I wasn't able to get the localization arguments working when using a JSON array of strings for APN, but it works when I use a regular array of strings. Can we JSON.parse the string to an array (which would not change the type signature) in the sendAPN.js file?

alex-friedl commented 3 years ago

Hello @eli-zhang. Thanks for reporting this! I will look into this as soon as I find some time

alex-friedl commented 3 years ago

@eli-zhang I think I understand the issue now, please correct me if I am mistaken:

When using FCM via node-gcm the expected payload is a JSON array as a String (https://github.com/ToothlessGear/node-gcm#notification-payload-option-table)
node-apn on the other hand expects an array which is why we can't use the same input type for both

So your proposed solution is to let the input type remain as string but parse that string to a JSON array in sendAPN and pass it as is in sendGCM ?

eli-zhang commented 3 years ago

Yep, that's correct!

I actually spun up a fork of the repo to have a working version until you got around to this issue. So far changing 'title-loc-args': data.titleLocArgs to 'title-loc-args': JSON.parse(data.titleLocArgs || "[]") and 'loc-args': data.locArgs || data.bodyLocArgs to 'loc-args': JSON.parse(data.locArgs || data.bodyLocArgs || "[]") in lib/sendAPN.js seems to have fixed the issue and the fix works properly for both Apple and Android.

It also doesn't break any type definitions in the @types/node-pushnotifications package!

alex-friedl commented 3 years ago

@eli-zhang Can you test https://github.com/appfeel/node-pushnotifications/pull/151 and let me know if it works as expected on your end?

eli-zhang commented 3 years ago

Tested and it works as expected! Thanks for resolving this. As a side note - when testing your changes I had to clone your repo and change the folder names because your src folder is included in .npmignore. Is this intentional and is there a better way to go about testing changes?

alex-friedl commented 3 years ago

@eli-zhang Thx for testing, I released the changes with v1.6.1

Not sure why the npmignore would influence the contents of the cloned directory? The src folder is included there because it is not needed for the published npm package but it should be included when you do a git clone