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

feat: automatically handle push click events and increase compatibility with 3rd party push modules #112

Merged
merged 16 commits into from
Feb 12, 2024

Conversation

levibostian
Copy link
Member

@levibostian levibostian commented Nov 16, 2023

Part of: https://github.com/customerio/issues/issues/11289

Goals of PR

Blocked by

Docs

PR

@@ -61,5 +61,5 @@ export function injectCodeByLineNumber(
content = [...lines.slice(0, index), snippet, ...lines.slice(index)];
}

return content;
return content.join('\n');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other injectN functions in this file return a string while this inject function returned an Array. I struggled using this function during development until I made this change.

@@ -62,7 +51,7 @@ const addImport = (stringContents: string, appName: string) => {
stringContents,
endOfMatchIndex,
addedImport
).join('\n');
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this after injectCodeByLineNumber now returns a string instead of Array.

package.json Outdated
@@ -40,7 +40,7 @@
"registry": "https://registry.npmjs.org/"
},
"peerDependencies": {
"customerio-reactnative": "^3.0.0"
"customerio-reactnative": ">=3.X.Y"
Copy link
Member Author

@levibostian levibostian Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be updated after new version of RN SDK gets deployed. The RN SDK will need to install newest version of iOS SDK that includes this PR.

These expo plugin modifications requires a minimum version of the RN SDK to be installed. That is what this change is trying to satisfy. I have not yet tested this syntax. I would expect npm to throw an error for anyone who is trying to install latest CIO expo plugin but using an older version of CIO RN SDK.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this syntax same as writing "customerio-reactnative": ">=3.0.0"? If you want to go with the above syntax then I think you should test it in an expo app before merging.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't understand what you're suggesting.

This PR is changing the file to >=3.X.Y (will change .X.Y after iOS feature released) and from your comment, it seems that you agree that >= should be used. Do you have a suggestion for using something else in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could have done a better job at explaining my previous comment. I understand that the version ranges >=3.X.Y and >=3.0.0 are equivalent, both indicating that any version of the package equal to or greater than 3.0.0 is acceptable and will be installed. Since you mentioned that you have not tested >=3.X.Y syntax, I wanted to verify with you if we are on the same page and the installation too gets tested before we release the fix.

I hope my explanation is more helpful this time.

@levibostian levibostian changed the base branch from beta to levi/fix-lint November 16, 2023 17:40
@levibostian levibostian requested a review from a team November 16, 2023 17:47
@levibostian levibostian marked this pull request as ready for review November 16, 2023 17:47
@levibostian levibostian marked this pull request as draft November 16, 2023 18:28
@@ -16,7 +16,11 @@ export type CustomerIOPluginOptionsIOS = {
appleTeamId?: string;
appName?: string;
disableNotificationRegistration?: boolean;
/**
* @deprecated No longer has any effect. Use autoTrackPushEvents to control if push metrics should be automatically tracked by SDK.
*/
handleNotificationClick?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleNotificationClick was an expo config plugin option because some customers would need to disable it if they were using another SDK that performs click handling (such as expo-notifications). That problem has been resolved by the new iOS feature and so there is no reason to disable the behavior.

handleNotificationClick?: boolean;
autoTrackPushEvents?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added new expo plugin config option which maps to the native iOS autoTrackPushEvents config option.

@levibostian levibostian marked this pull request as ready for review November 16, 2023 19:53
Base automatically changed from levi/fix-lint to beta November 17, 2023 14:17
Copy link
Contributor

@Shahroz16 Shahroz16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added my thoughts and it looks okay, but would appreciate more eyes from folks with expo experience.

src/helpers/constants/ios.ts Outdated Show resolved Hide resolved
@@ -11,6 +11,18 @@ public class CIOAppPushNotificationsHandler : NSObject {

{{REGISTER_SNIPPET}}

@objc(initializeCioSdk)
public func initializeCioSdk() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the expo utilized React native SDK initialization as well, can that be an issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we do suggest Expo customers to initialize the SDK via RN SDK.

The only issue that I am aware of was a memory issue that was caused by native initialize() being called multiple times in a short amount of time. That issue did receive a bug fix recently that should prevent that from happening.

@levibostian levibostian changed the title fix: improve reliability of auto push metrics and deep link handling feat: automatically handle push click events and increase compatibility with 3rd party push modules Feb 12, 2024
PR is failing to run npm ci. Hoping this fixes that.
@levibostian levibostian force-pushed the levi/reliable-open-metrics-not-a-prototype branch from c69fced to ddcaa7e Compare February 12, 2024 17:38
@levibostian levibostian merged commit a67e345 into beta Feb 12, 2024
3 checks passed
@levibostian levibostian deleted the levi/reliable-open-metrics-not-a-prototype branch February 12, 2024 17:39
github-actions bot pushed a commit that referenced this pull request Feb 12, 2024
## [1.0.0-beta.14](1.0.0-beta.13...1.0.0-beta.14) (2024-02-12)

### Features

* automatically handle push click events and increase compatibility with 3rd party push modules ([#112](#112)) ([a67e345](a67e345))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Push deep links not working when using multiple push notification modules (ie expo-notifications)
3 participants