fix: include required messagingpush dependency for no push configuration #187

levibostian · 2023-08-18T18:30:14Z

The RN SDK today does require the messagingpush SDK module but the podspec that we ask customers to install does not include this module. This could mean compile-time errors.

Yes, we do expect that customers setup rich push in their app and so we do not expect customers to encounter this problem. However, the idea that this PR also adds a comment to the podspec files specifying required dependencies is a good value add to the codebase.

PR in draft mode and asks for review by team to get feedback if this is something we want to do or not.

Note: Conversation that sparked this idea: https://github.com/customerio/docs/pull/1446#discussion_r1298239891

https://github.com/customerio/docs/pull/1446#discussion_r1298239891

github-actions · 2023-08-18T18:30:28Z

Pull request title looks good 👍!

If this pull request gets merged, it will cause a new release of the software. Example: If this project's latest release version is 1.0.0. If this pull request gets merged in, the next release of this project will be 1.0.1. This pull request is not a breaking change.

All merged pull requests will eventually get deployed. But some types of pull requests will trigger a deployment (such as features and bug fixes) while some pull requests will wait to get deployed until a later time.

This project uses a special format for pull requests titles. Expand this section to learn more (expand by clicking the ᐅ symbol on the left side of this sentence)...

This project uses a special format for pull requests titles. Don't worry, it's easy!

This pull request title should be in this format:

<type>: short description of change being made

If your pull request introduces breaking changes to the code, use this format:

<type>!: short description of breaking change

where <type> is one of the following:

feat: - A feature is being added or modified by this pull request. Use this if you made any changes to any of the features of the project.
fix: - A bug is being fixed by this pull request. Use this if you made any fixes to bugs in the project.
docs: - This pull request is making documentation changes, only.
refactor: - A change was made that doesn't fix a bug or add a feature.
test: - Adds missing tests or fixes broken tests.
style: - Changes that do not effect the code (whitespace, linting, formatting, semi-colons, etc)
perf: - Changes improve performance of the code.
build: - Changes to the build system (maven, npm, gulp, etc)
ci: - Changes to the CI build system (Travis, GitHub Actions, Circle, etc)
chore: - Other changes to project that don't modify source code or test files.
revert: - Reverts a previous commit that was made.

Examples:

feat: edit profile photo
refactor!: remove deprecated v1 endpoints
build: update npm dependencies
style: run formatter

Need more examples? Want to learn more about this format? Check out the official docs.

Note: If your pull request does multiple things such as adding a feature and makes changes to the CI server and fixes some bugs then you might want to consider splitting this pull request up into multiple smaller pull requests.

lj-cio · 2023-08-18T19:28:05Z

customerio-reactnative.podspec

-    # no dependencies. This is the default subspec designed to not install any push dependencies.
+    # This is the default subspec designed to not install any push dependencies. Customer should choose APN or FCM.
+    # The SDK at runtime currently requires the MessagingPush module so we do include it here. 
+    ss.dependency "CustomerIO/MessagingPush", package["cioNativeiOSSdkVersion"]


Looking at the comment, I'm not clear on what the customer needs to do. What about adding both APN and FCM options as comments and directing the customer to uncomment the one they need and deleting the other?

I'm not clear on what the customer needs to do.

The customer needs to follow our SDK docs to setup APN or FCM push.

This PR is trying to handle the scenario where a customer has not yet setup APN or FCM push. Some customers have sent us support tickets saying that they are experiencing issues with our SDK while following our SDK docs. They must be doing 1 step at a time, continuously compiling their app and testing along the way. If a customer does that, they could encounter issues. This PR is to try and fix those issues.

This PR does not effect our docs today. No work on customers part besides what they already do.

Shahroz16

I like the idea but added some thoughts

Shahroz16 · 2023-08-21T09:27:05Z

customerio-reactnative.podspec

@@ -28,7 +28,9 @@ Pod::Spec.new do |s|
  s.default_subspec = "nopush"

  s.subspec 'nopush' do |ss|


Maybe we need to move away from wording nopush because otherwise, it can get confusing. how about base/common ?

This name would be for internal purposes, only which is why I included docs into this file explaining it. However, I do understand what you mean by nopush being confusing.

I wouldn't mind a word generic-push or base-push which implies that the RN SDK does require some sort of push setup, but it's not specific to the service yet.

Sweet, I am approving it as this isn't a blocker, feel free to update it to a more generic name now or later.

Shahroz16 · 2023-08-22T09:09:41Z

customerio-reactnative.podspec

@@ -28,7 +28,9 @@ Pod::Spec.new do |s|
  s.default_subspec = "nopush"

  s.subspec 'nopush' do |ss|


Sweet, I am approving it as this isn't a blocker, feel free to update it to a more generic name now or later.

gerjunior-groundswell · 2023-08-25T15:56:19Z

Hey guys!
Any updates on when this is going to be merged?

### [3.1.8](3.1.7...3.1.8) (2023-08-28) ### Bug Fixes * include required messagingpush dependency for no push configuration ([#187](#187)) ([78bbc63](78bbc63))

levibostian · 2023-08-28T17:12:45Z

@gerjunior-groundswell, this has been deployed in 3.1.8. Thank you for your patience while we test the change before merging. Enjoy!

build: include required messagingpush dependency

1a29e90

https://github.com/customerio/docs/pull/1446#discussion_r1298239891

levibostian requested a review from a team August 18, 2023 18:40

lj-cio reviewed Aug 18, 2023

View reviewed changes

Shahroz16 reviewed Aug 21, 2023

View reviewed changes

levibostian marked this pull request as ready for review August 21, 2023 20:02

levibostian requested review from lj-cio and Shahroz16 August 21, 2023 20:02

Shahroz16 approved these changes Aug 22, 2023

View reviewed changes

levibostian changed the title ~~build: include required messagingpush dependency~~ fix: include required messagingpush dependency Aug 22, 2023

levibostian changed the title ~~fix: include required messagingpush dependency~~ fix: include required messagingpush dependency for no push configuration Aug 22, 2023

mrehan27 approved these changes Aug 23, 2023

View reviewed changes

mrehan27 mentioned this pull request Aug 25, 2023

No such module 'CioMessagingPush' #189

Closed

levibostian merged commit 78bbc63 into main Aug 28, 2023

levibostian deleted the levibostian-patch-1 branch August 28, 2023 17:07

github-actions bot pushed a commit that referenced this pull request Aug 28, 2023

chore: prepare for 3.1.8

77b60d7

### [3.1.8](3.1.7...3.1.8) (2023-08-28) ### Bug Fixes * include required messagingpush dependency for no push configuration ([#187](#187)) ([78bbc63](78bbc63))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: include required messagingpush dependency for no push configuration #187

fix: include required messagingpush dependency for no push configuration #187

levibostian commented Aug 18, 2023 •

edited

Loading

github-actions bot commented Aug 18, 2023 •

edited

Loading

Examples:

lj-cio Aug 18, 2023

levibostian Aug 21, 2023

Shahroz16 left a comment

Shahroz16 Aug 21, 2023

levibostian Aug 21, 2023

Shahroz16 Aug 22, 2023

Shahroz16 Aug 22, 2023

gerjunior-groundswell commented Aug 25, 2023

levibostian commented Aug 28, 2023

		@@ -28,7 +28,9 @@ Pod::Spec.new do \|s\|
		s.default_subspec = "nopush"

		s.subspec 'nopush' do \|ss\|

fix: include required messagingpush dependency for no push configuration #187

fix: include required messagingpush dependency for no push configuration #187

Conversation

levibostian commented Aug 18, 2023 • edited Loading

github-actions bot commented Aug 18, 2023 • edited Loading

Examples:

lj-cio Aug 18, 2023

Choose a reason for hiding this comment

levibostian Aug 21, 2023

Choose a reason for hiding this comment

Shahroz16 left a comment

Choose a reason for hiding this comment

Shahroz16 Aug 21, 2023

Choose a reason for hiding this comment

levibostian Aug 21, 2023

Choose a reason for hiding this comment

Shahroz16 Aug 22, 2023

Choose a reason for hiding this comment

Shahroz16 Aug 22, 2023

Choose a reason for hiding this comment

gerjunior-groundswell commented Aug 25, 2023

levibostian commented Aug 28, 2023

levibostian commented Aug 18, 2023 •

edited

Loading

github-actions bot commented Aug 18, 2023 •

edited

Loading