Skip to content

fix: compatibility with react native 0.76#359

Merged
mrehan27 merged 5 commits intomainfrom
rehan/mbl-642-rn-76-support
Nov 20, 2024
Merged

fix: compatibility with react native 0.76#359
mrehan27 merged 5 commits intomainfrom
rehan/mbl-642-rn-76-support

Conversation

@mrehan27
Copy link
Copy Markdown
Contributor

@mrehan27 mrehan27 commented Nov 18, 2024

fixes: MBL-642

Changes

  • Added consistent objc annotations to all Swift methods used by Objective-C classes
  • Formatted Swift code using the same formatter as native iOS codebase
  • Formatted Objective-C code using online formatter for consistency

Tested On

  • Current APN sample app (compiled and ran without errors)
  • New sample app with React Native 0.76 (compiled and ran without errors)

@mrehan27 mrehan27 self-assigned this Nov 18, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 18, 2024

Sample app builds 📱

Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.


  • FCM: 359.5.0 (28868373)
  • APN: 359.5.0 (28868372)

Base automatically changed from rehan/mbl-642-lock-file-updates to main November 18, 2024 20:15
@mrehan27 mrehan27 force-pushed the rehan/mbl-642-rn-76-support branch from 4f00692 to c2a94cc Compare November 18, 2024 20:28
@mrehan27 mrehan27 force-pushed the rehan/mbl-642-rn-76-support branch from c2a94cc to 5118d6f Compare November 18, 2024 20:34
@mrehan27 mrehan27 requested a review from Shahroz16 November 19, 2024 06:04
Copy link
Copy Markdown
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.

changes looks okay, i am little unsure about what the issue was with the support for 0.76. Does 0.76 require these objc anatotions now that we added because I believe with Turbo and everything they are probably moving away.

If #352 solves the issue with compatibility, do we still need to add additional anotations specifically on methods?

@mrehan27
Copy link
Copy Markdown
Contributor Author

@Shahroz16 The goal of this ticket was not to specifically include Turbo Modules but rather to ensure that our SDK compiles and runs with latest RN version (0.76) to unblock customers from using it. The PR you mentioned does address the issue, but the additional changes were added to ensure code consistency and to avoid any potential similar issues in the future.

Turbo Modules weren't expected to be covered under this ticket and may be addressed in a separate ticket/project, as implementing them might require more effort and a broader review of our SDK before going live with it.

@mrehan27 mrehan27 requested a review from Shahroz16 November 19, 2024 14:07
@Shahroz16
Copy link
Copy Markdown
Contributor

@Shahroz16 The goal of this ticket was not to specifically include Turbo Modules but rather to ensure that our SDK compiles and runs with latest RN version (0.76) to unblock customers from using it. The PR you mentioned does address the issue, but the additional changes were added to ensure code consistency and to avoid any potential similar issues in the future.

Turbo Modules weren't expected to be covered under this ticket and may be addressed in a separate ticket/project, as implementing them might require more effort and a broader review of our SDK before going live with it.

I agree, the scope of this ticket didn't include Turbo, and that is not where my question is at either. Let me rephrase.

The obj-c annotation, that you added, they were in the code before too. But they were removed, because React native doesn't fully require explicit obj-c annotation as it moving towards "Turbo and bridgeless approach". So these annotations were removed in the data pipeline release.

The fix for the support for 0.76 requires parameter name changes done in the other PR, so the annotation doesn't provide any benefit here but actually takes us back. If you think these annotations are necessary for the compatibility with 0.76, then I am okay with adding them back, but if not then, we should probably not add them.

So, I would suggest probably, revert those, and keep the formating because we would need a fix PR for the release.

@mrehan27
Copy link
Copy Markdown
Contributor Author

Thanks for sharing the details @Shahroz16. I wasn't fully aware of these changes in our SDK or the updates in React Native. I'm completely fine reverting the annotations, but I have a few questions I think we should consider before proceeding:

  • If I'm not mistaken, this change was introduced in RN 0.73. By reverting to simple @objc annotation, will the SDK still work for customers using older versions of React Native? If not, does this mean current v4 could already be causing issues for customers on versions older than 0.73? What do you think the SDK behavior would be in those cases?
  • Are these changes optional? If the annotations were updated purely because RN no longer require them now, perhaps we could consider keeping them to ensure compatibility with older versions of React Native? Unless reverting them still works seamlessly for older versions. If you believe it should work fine for older versions as well, I'm okay reverting this.

Let me know what you think so I can update PR accordingly!

@Shahroz16
Copy link
Copy Markdown
Contributor

  • If I'm not mistaken, this change was introduced in RN 0.73. By reverting to simple @objc annotation, will the SDK still work for customers using older versions of React Native? If not, does this mean current v4 could already be causing issues for customers on versions older than 0.73? What do you think the SDK behavior would be in those cases?

I don't think that is the case. This flexibility has been there before, too. What you are referring to is a bridgeless concept introduced in 0.73; it was added to support Turbo modules. I think that's the reason no one has complained yet about v4 not working for them. Because we are not removing annotations, but rather parameters. For example, @objc instead of @objc(identify:traits:)

The issue we face right now is what format Turbo modules expect the bridges to be in. This is why, regardless of whether we changed the annotations in the previous pattern compared to this, it would not have worked.

@mrehan27
Copy link
Copy Markdown
Contributor Author

@Shahroz16 Thanks for clarifying. I'll then go ahead and revert all the annotation changes.

@mrehan27 mrehan27 changed the title fix: support for react native 0.76 fix: compatibility with react native 0.76 Nov 20, 2024
@mrehan27 mrehan27 merged commit 6fcdd91 into main Nov 20, 2024
@mrehan27 mrehan27 deleted the rehan/mbl-642-rn-76-support branch November 20, 2024 13:34
github-actions Bot pushed a commit that referenced this pull request Nov 20, 2024
## [4.1.1](4.1.0...4.1.1) (2024-11-20)

### Bug Fixes

* compatibility with react native 0.76 ([#359](#359)) ([6fcdd91](6fcdd91))
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.

2 participants