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

Update the iOS template from Obj-C to Swift #41896

Closed
wants to merge 3 commits into from

Conversation

TheRogue76
Copy link
Contributor

Summary:

Before this PR gets controversial, i just want to preface it by saying that i understand that moving EVERYTHING over from Obj-C to Swift is not viable, and I understand why it has not been done till now, but i think we can have a nice path forward if we just update the bits that are facing OSS, and leave everything else for later.

As mentioned in react-native-community/discussions-and-proposals#253 (comment) Apple will enforce Xcode 15 (Swift 5.9) from around April of 2024 for sending new apps to appstore connect, so the stuff we will need to add support for directly wiring up the C++ sections of RN to the Swift parts can be migrated soon, but that is not the scope of this PR.

This PR is inspired by the recent migration of the Android Template from Java to Kotlin. In that migration, we updated the tests, and the primary template, but we left everything else untouched.

The reasoning is also similar to that of the Android migration: Most developers would prefer to create the native bits of their apps using more modern languages. These languages are easier to debug and problems in them can be discovered much quicker.

The update to the template also ensures the users who have done these migrations on their own that they can still use the react native upgrade helper to update their apps every time a new version of React Native is released.

The end goal is to at least have a conversation, and see how viable it is to do this. I tried this on a test repository before hand to make sure it worked fine in both debug mode and release

Changelog:

[IOS] [BREAKING] - update the iOS app template from Objective-C to Swift. Also add the DEBUG Active compilation conditions flag for the Swift compiler so:

#if DEBUG
return RCTBundleURLProvider.sharedSettings().jsBundleURL(forBundleRoot: "index")
#else
return Bundle.main.url(forResource: "main", withExtension: "jsbundle")!
#endif

is read correctly.

Test Plan:

My test repository:
https://github.com/TheRogue76/React-Native-Template-With-Swift
Screenshot:
Screenshot 2023-12-11 at 19 44 36
And the CI being green

@facebook-github-bot
Copy link
Contributor

Hi @TheRogue76!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@analysis-bot
Copy link

analysis-bot commented Dec 11, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 16,517,133 -3,655
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 19,895,948 +462
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 86df742
Branch: main

@TheRogue76
Copy link
Contributor Author

The failed test had simply timed out. And i signed the CLA. Let's hope it picks it up correctly.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Dec 11, 2023
@ryancat
Copy link
Contributor

ryancat commented Dec 11, 2023

Does anything depend on the hello world template being created in Obj-C? I think it's a good approach. Could you summarize a bit more on what might break and how to mitigate the risk?

@TheRogue76
Copy link
Contributor Author

TheRogue76 commented Dec 11, 2023

Does anything depend on the hello world template being created in Obj-C? I think it's a good approach. Could you summarize a bit more on what might break and how to mitigate the risk?

I don't believe it does, since it is the top of the app and since it consumes everything, any dependency on it might cause a dependency cycle, but i also admit i am not aware of all the details on the iOS side of the repo (Please feel free to add anyone you think might know more). I did double check that the bundle URLs were the correct values for debug/release. Only things i felt were noteworthy were:

  1. If you want to use #if DEBUG in Swift, the flavour that you are targeting needs to have that flag added to it for the Swift Compiler (see here for an example of me adding it. This will be important if you have a lot of flavours of the app and want some of them to be run in debug mode and some in release mode). The mitigation could be a simple doc update since it is not a case everyone has, and this update to the template should cover the normal userbases use-case.
  2. In order for Swift and Obj-C to work next together nicely, you will need a Bridging header file, included in the PR. I am not sure how the template renames the files from HelloWorld to whatever the CLI input is when creating the project, but if there is anything specific, it should consider renaming this bit as well.

The imports might look a bit weird (For example React_RCTAppDelegate which in ObjC land is #import <RCTAppDelegate.h> and gives you access to RCTAppDelegate) but that shouldn't be too much of a problem

I tried to keep the structure of files the same as the original (AppDelegate.mm/h -> AppDelegate.swift, main.m -> main.swift) but the truth is that it is not really needed. Simply annotating the AppDelegate with @UIApplicationMain is enough I decided to get rid of the main file and use @main. It was introduced in Swift 5.3 (Xcode 12), and if we are migrating, might as well use all the sweet stuff that come with it

@TheRogue76
Copy link
Contributor Author

TheRogue76 commented Dec 11, 2023

Screenshot 2023-12-11 at 22 57 23

This is a picture of where that value needs to be added in the Swift compiler flags in XCode

@@ -0,0 +1,8 @@
import UIKit

UIApplicationMain(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed, you can annotate AppDelegate with @main. Check newly generated project from Xcode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved under 92a1ae0. I wanted to keep the file structures the same for review, but if we are committing to use Swift, might as well take advantage of it. And @main was introduced in 5.3, so pretty much everyone should have it.

@cipolleschi
Copy link
Contributor

Hi @TheRogue76, thanks for submitting the PR.
I haven't had the time to test it properly, as I came back from PTO yesterday and I have to catch up several other things.

On the technical side, I can tell you already that there are several things missing: for example, there are no changes to the cocoapod infra to pass over all the Cxx flags that we pass and we would have to pass them to Swift as well, in the form of "Active Compilation Condition" and OTHER_SWIFT_FLAGS with the -Xcc modifier before them.
Also, the approach of modifying the pbxproj is discouraged, as it is very hard to maintain and to update. Instead, we can handle all those changes internally, modifying the cocoapods scripts.

I don't want to discourage you, as this is an amazing starting point.
React Native ecosystem is huge and we need to keep multiple things into consideration.

Context

To share more context on the complexity of this change, changing from Objective-C to Swift is not as straightforward as from Java to Kotlin, unfortunately.
Kotlin offer 1-o-1 support for all the Java features, but Swift and Objective-C (and worse Objective-C++) are two completely different languages.

I tried myself the migration a couple of time in the past and always encountered issues which would have taken a non negligible amount of time to be fixed.
The most relevant ones:

  • not all the feature that we need to from C++ are supported by the Swift-C++ interop. For example, to create a custom component in the New Architecture, you'll have to subclass a virtual C++ class and that can't be done in Swift as of today
  • the internal structure of ReactCommon is not a Clang module. As soon as a Swift library need to import some file from ReactCommon, it won't build anymore.

Moreover, there are several subtleties that we need to consider. Just to mention some of them:

  • Template must work with the Old Architecture
  • Template must work with the New Architecture, with Bridge.
  • Template must work with the New Architecture, with Bridgeless.
  • We need to coordinate with Expo to make sure not to break their config-plugin (and if we can't mitigate that, at least let them know).
  • This change in the template will break the integration with libraries that requires to modify the App Delegate. Moving to Swift will "break" at least all the READMEs for those libraries. Therefore, we need to provide some form of migration documentation.

Talking about the auxiliary tools:

  • We have to make sure the CLI still works with Swift files, otherwise we can't create a working project
  • We need to check how it will look like in the upgrade helper and if we can do something to make it better looking than a "remove objc files/add the new swift file".
  • we need to update all the documentation in the website.

Next Steps

So, yeah, although this is a great starting point, I think that a better approach would be to:

  1. revamp the "Transition to Swift" RFC, probably rewrite it
  2. list all the required changes, including all the tooling. This will help all the partners (Microsoft, Expo, Callstack...) to chime in and make sure we don't forget about anything
  3. sequence all the changes with "tasks"
  4. once we get approval from everyone, we can start with the implementation, with more insurance that it will end up well.

@TheRogue76
Copy link
Contributor Author

Hey @cipolleschi

All fair. I assumed that since the Template itself doesn't interact directly with those bits (just RCTDelegate) and is simply there for the consumption of OSS, we could simply circumvent the issue altogether and transition over the delegate itself, but if it's going to propagate I suppose that won't work.

What sort of help can I provide on the RFC side?

@cipolleschi
Copy link
Contributor

I haven't read the RFC yet, but as I can see is from 2020... and multiple things have changed since then! 😄

So, I'd probably start by taking the RFC and by rewriting it. This will help to remove the bits that are not valid anymore and to add all the considerations I added above.

The second part of the RFC would list all the changes (at a high-level) that we need to apply:

  • changes in the iOS files
  • changes in the cocoapods infra
  • changes in the doc
  • changes in the CLI
  • changes in the update helper.

This helps in identifying dependencies and make sure we can involve all the people that might have opinions or knowledge about it.

The last part should be related to Adoption and Communication: what users should expect when a new version of React Native with this changes is released. This probably implies writing a migration guide from Obj-C to Swift.

As a reference for the RFC structure, you can take the Wide Gamut RFC.

Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jun 12, 2024
Copy link

This PR was closed because it has been stalled for 7 days with no activity.

1 similar comment
Copy link

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Jun 19, 2024
@github-actions github-actions bot closed this Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants