Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ios): make sure notificationMessage is immutable #317

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Conversation

erisu
Copy link

@erisu erisu commented Nov 28, 2024

Description

Update to make sure that notificationMessage is correctly storing an NSDictionary.

Create a mutable copy when the content needs to be updated and then restore an immutable copy.

Related Issue

Resolves #314
Closes #315

Motivation and Context

Fix issue reported in #314 to prevent app crashes.

How Has This Been Tested?

Deployed app to iOS 18.1 device.

Pushed payload from AWS Pinpoint:

{
  "APNSMessage": {
    "aps": {
      "alert": "Sample message for iOS endpoints",
      "content-available": "1"
    }
  }
}

Pushed payload from Amazon SNS

{
  "APNS_SANDBOX": "{\"aps\":{\"alert\":\"Sample message for iOS development endpoints\", \"content-available\": \"1\"}}"
}

Push plugin was initialized with the following:

    const push = PushNotification.init({
        ios: {
            alert: true,
            badge: true,
            sound: true
        }
    });

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@erisu erisu added this to the 5.0.1 milestone Nov 28, 2024
@erisu erisu added the ios Related to the iOS platform label Nov 28, 2024
@geoidesic
Copy link

I tried installing this commit to test it but:

cordova plugin add https://github.com/havesource/cordova-plugin-push/pull/317/commits/0117971a75a56cd4b1ac20af8a205a5aac11903b
Failed to fetch plugin https://github.com/havesource/cordova-plugin-push/pull/317/commits/0117971a75a56cd4b1ac20af8a205a5aac11903b via registry.
Probably this is either a connection problem, or plugin spec is incorrect.
Check your connection and plugin name/version/URL.
CordovaError: Error: TAR_BAD_ARCHIVE: Unrecognized archive format

@erisu
Copy link
Author

erisu commented Nov 30, 2024

@geoidesic I don't think the provided method is a valid way to check out a plugin. It should follow a pattern that npm understands.

I recommend using the following command to check out the PR:

cordova plugin add github:havesource/cordova-plugin-push\#fix/GH-314

@geoidesic
Copy link

@erisu thx for that. I was able to install that version. It does fix the crash. However, the foreground property is NULL?

{
    "additionalData": {
        "message": "Carl Hoing",
        "matchID": 27466,
        "matchOpCode": "trlxo6bsnxxz83in7cuphycsbabqcmc5lmom5oqiqdfaymuctjfn3y9bgxt5twb5",
        "foreground": "",
        "coldstart": "",
        "matchKey": "ogmhf6j1vexgngce2clqepnl4cedarid"
    },
    "message": "bump",
    "title": "Carl Hoing",
    "count": 7,
    "sound": "default"
}

@erisu
Copy link
Author

erisu commented Dec 1, 2024

This PR is unrelated to foreground or coldstart.

It only addresses the app crash caused by attempting to edit an immutable object.

You'll need to open a new issue ticket and provide detailed information about how you're configuring the PushNotification.init settings for your app, as well as the payload you're sending. You may also want to review your JS code, because if the foreground or coldstart key exists, it can only be true or false based on what I see in the native code.

This PR and the original issue ticket are not the appropriate places to discuss additional issues.

I will merge as fixed.

@geoidesic
Copy link

Right. I notice I was wrong about the NULL value, that was a transformation error in my logging tool.

@erisu erisu merged commit b939abc into master Dec 2, 2024
7 checks passed
@erisu erisu deleted the fix/GH-314 branch December 2, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ios Related to the iOS platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App crashes when activating Push Notification, after upgrading to v5 iOS
2 participants