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 using Swift in a native module with Fabric enabled #33743

Closed
wants to merge 1 commit into from

Conversation

fortmarek
Copy link
Contributor

@fortmarek fortmarek commented May 2, 2022

Summary

Using Fabric with a Swift native module is currently broken. There are currently two issues.

If you try to integrate a native module with Swift code, you will get the following error when running pod install with Fabric enabled:

[!] The following Swift pods cannot yet be integrated as static libraries:

The Swift pod `MyNativeView` depends upon `React-RCTFabric`, `React-Codegen`, `RCTTypeSafety`, and `ReactCommon`, which do not define modules. 
To opt into those targets generating module maps (which is necessary to import them from Swift when building as static libraries), 
you may set `use_modular_headers!` globally in your Podfile, or specify `:modular_headers => true` for particular dependencies.

To resolve this, I have applied the suggestion from the error (set :modular_headers => true for the appropriate modules inside react_native_pods.rb.

Afterwards, pod install succeeds but I still got Redefinition of module 'React' during the build due to the conflict inside the generated modulesmaps React-Core.modulemap and React-RCTFabric.modulemap. This makes sense since React-RCTFabric.podspec has s.header_dir = "React" (see here) and the module inherits that. However, we can explicitly specify module_name for the podspec which is what I have done. I have named the module Fabric, let me know if you think there's a better name.

Changelog

[iOS] [Fixed] - Fix using Swift in a native module with Fabric enabled

Test Plan

  1. Clone this repo
  2. From main, apply changes from this commit (adding Swift file to the MyNativeView native module in the RN tester app)
  3. Try to run USE_FABRIC=1 RCT_NEW_ARCH_ENABLED=1 USE_CODEGEN_DISCOVERY=1 USE_HERMES=0 bundle exec pod install inside the packages/rn-tester
  4. Observe errors
  5. Apply the commit from this PR
  6. Both pod install and the subsequent build should succeed.

I can also make changes to the current MyNativeView module to include Swift as well if it's something that the React Native Core team would be interested in - in case you want the Swift native modules to be always buildable on main

@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. p: Shopify Partner: Shopify Partner labels May 2, 2022
@fortmarek fortmarek requested review from cipolleschi and cortinico May 2, 2022 13:10
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label May 2, 2022
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels May 2, 2022
@analysis-bot
Copy link

analysis-bot commented May 2, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,775,912 -662
android hermes armeabi-v7a 7,181,245 -593
android hermes x86 8,084,537 -803
android hermes x86_64 8,065,242 -375
android jsc arm64-v8a 9,650,077 +36
android jsc armeabi-v7a 8,424,169 +117
android jsc x86 9,599,556 +112
android jsc x86_64 10,197,138 +286

Base commit: e7d9e4d
Branch: main

@analysis-bot
Copy link

analysis-bot commented May 2, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: e7d9e4d
Branch: main

@cipolleschi
Copy link
Contributor

Hi @fortmarek, thank you for the PR.

The code looks fine to me, I will call the module RCTFabric, just to avoid misunderstanding with the React-Fabric pod which is installed as a Development Pod in the new architecture.

Just to make it clear, at the moment RN does not support Swift, as explained here. But I think that your PR will solve the issue of the user who posted (and then edited) the question.

I'm a bit puzzled about a few things, and I'd like to understand their rationale:

  1. In the commit you suggest to apply, we are introducing a new flag RCT_FABRIC_ENABLED. Then, we use it to enable the code of MyNativeView when Fabric is enabled and to disable it when it's not. Why? Couldn't we use the RCT_NEW_ARCH_ENABLED flag and avoid adding another flag that we will have to maintain?
  2. In the test plan, you specified USE_HERMES=0. Going forward, we would like to push Hermes as the default JS engine.
    1. Why did you tested it with that setting?
    2. What happens if we use Hermes?

Additionally, as highlighted here, we can drop the USE_CODEGEN_DISCOVERY=1 flag requirement, given that the RCT_NEW_ARCH_ENABLED=1 turns the other one on and it is not used anymore.

@fortmarek fortmarek force-pushed the fix/swift-fabric branch from 9772c62 to 214ed81 Compare May 3, 2022 07:25
@fortmarek
Copy link
Contributor Author

Thanks for the review @cipolleschi 🙌 I have updated the module name to RCTFabric, so the PR should be ok to merge now.

Just to make it clear, at the moment RN does not support Swift, as explained reactwg/react-native-new-architecture#34 (comment).

Yes, I am aware you cannot use Swift to directly write component views. But you can still write Swift views and leverage them in the Objective-C++ code.

In the commit you suggest to apply, we are introducing a new flag RCT_FABRIC_ENABLED. Then, we use it to enable the code of MyNativeView when Fabric is enabled and to disable it when it's not. Why? Couldn't we use the RCT_NEW_ARCH_ENABLED flag and avoid adding another flag that we will have to maintain?

Right, I definitely can, the example was maybe put together too hastily as it was not supposed to be merged 🙂 thanks for the suggestion 👍

In the test plan, you specified USE_HERMES=0. Going forward, we would like to push Hermes as the default JS engine.

On main and 0.69.0-rc.0, installing hermes-engine takes quite a long time as reported here. As I did not care about the engine and wanted to get the example done quickly, I just chose JSC.

Additionally, as highlighted here, we can drop the USE_CODEGEN_DISCOVERY=1 flag requirement, given that the RCT_NEW_ARCH_ENABLED=1 turns the other one on and it is not used anymore.

Thanks, I was not aware of that! Still new to the Fabric arch ^^

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@mateusz1913
Copy link
Contributor

@fortmarek Thx for this PR! It almost resolves discussion I started here. I tried your change in my Turbomodule poc & it nearly worked, the only Pod which still failed was RCTFolly. However, after I added :modular_headers => true locally to RCTFolly line 81, issue was resolved. Could you add that fix for RCTFolly pod as well, so both Fabric & Turbomodule use cases will be fixed?

@fortmarek fortmarek force-pushed the fix/swift-fabric branch from 214ed81 to 50674ac Compare May 4, 2022 06:52
@fortmarek
Copy link
Contributor Author

Could you add that fix for RCTFolly pod as well, so both Fabric & Turbomodule use cases will be fixed?

Hey @mateusz1913, I don't see a reason why not to add it. Afaict, it should be fine and unblock you as well cc @cipolleschi

@cipolleschi
Copy link
Contributor

All good, I agree with the changes. I will reimport the PR!

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @fortmarek in 709a459.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label May 4, 2022
fortmarek added a commit that referenced this pull request May 11, 2022
Summary:
Using Fabric with a Swift native module is currently broken. There are currently two issues.

If you try to integrate a native module with Swift code, you will get the following error when running `pod install` with Fabric enabled:
```
[!] The following Swift pods cannot yet be integrated as static libraries:

The Swift pod `MyNativeView` depends upon `React-RCTFabric`, `React-Codegen`, `RCTTypeSafety`, and `ReactCommon`, which do not define modules.
To opt into those targets generating module maps (which is necessary to import them from Swift when building as static libraries),
you may set `use_modular_headers!` globally in your Podfile, or specify `:modular_headers => true` for particular dependencies.
```

To resolve this, I have applied the suggestion from the error (set `:modular_headers => true` for the appropriate modules inside `react_native_pods.rb`.

Afterwards, `pod install` succeeds but I still got `Redefinition of module 'React'` during the build due to the conflict inside the generated modulesmaps  `React-Core.modulemap` and `React-RCTFabric.modulemap`. This makes sense since `React-RCTFabric.podspec` has `s.header_dir = "React"` (see [here](https://github.com/facebook/react-native/blob/main/React/React-RCTFabric.podspec#L37)) and the module inherits that. However, we can explicitly specify `module_name` for the podspec which is what I have done. I have named the module `Fabric`, let me know if you think there's a better name.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - Fix using Swift in a native module with Fabric enabled

Pull Request resolved: #33743

Test Plan:
1. Clone [this](https://github.com/fortmarek/react-native) repo
2. From `main`, apply changes from [this](fortmarek@26958fc) commit (adding Swift file to the `MyNativeView` native module in the RN tester app)
3. Try to run `USE_FABRIC=1 RCT_NEW_ARCH_ENABLED=1 USE_CODEGEN_DISCOVERY=1 USE_HERMES=0 bundle exec pod install` inside the `packages/rn-tester`
4. Observe errors
5. Apply [the commit](9772c62) from this PR
6. Both pod install and the subsequent build should succeed.

I can also make changes to the current `MyNativeView` module to include Swift as well if it's something that the React Native Core team would be interested in - in case you want the Swift native modules to be always buildable on `main`

Reviewed By: dmitryrykun

Differential Revision: D36097852

Pulled By: cipolleschi

fbshipit-source-id: 2faebcffd1115339f89a406e265a6a040218dc9c
@andrestone
Copy link
Contributor

andrestone commented May 18, 2022

Hey @fortmarek! Thanks for this and the upcoming 0.69 release.

I can also make changes to the current MyNativeView module to include Swift as well if it's something that the React Native Core team would be interested in - in case you want the Swift native modules to be always buildable on main

I was wondering if this process is simple enough so we could have a flag for it. I tried the usual process (setting up bridging headers) on RNTester but it didn't work. Would mind enlightening me on this?

Never mind, I had a typo on my import 🤦🏻

Cheers.

@fortmarek
Copy link
Contributor Author

Glad that you found out the issue @andrestone 🙌

@BraveEvidence
Copy link

@fortmarek Will this be part of RN 0.69 release?

@fortmarek
Copy link
Contributor Author

Will this be part of RN 0.69 release?

Yes, you can find all the picks for 0.69 here

@fortmarek
Copy link
Contributor Author

Hey man just out of curiosity i am able to use swift module with fabric enabled in RN 0.68.2. I am not sure what exactly is the issue this PR going to solve.

This PR aims to enable you to use Swift pods library - I have not been trying to use an internal Swift module. I'd say that might be the difference. I'd suggest checking out the test plan to understand better the use-case.

facebook-github-bot pushed a commit that referenced this pull request Jun 30, 2023
Summary:
This fixes a bug that started with React Native 0.72.0 when using the new architecture and installing a native lib that has Swift code (in my case, `datadog/mobile-react-native`).

Running `pod install` errors with the following output (`DatadogSDKReactNative` is the pod containing the Swift code):

```
[...]
Analyzing dependencies
Downloading dependencies
Installing DatadogSDKReactNative 1.8.0-rc0
[!] The following Swift pods cannot yet be integrated as static libraries:

The Swift pod `DatadogSDKReactNative` depends upon `React-Fabric`, `React-graphics`, `React-utils`, and `React-debug`, which do not define modules. To opt into those targets generating module maps (which is necessary to import them from Swift when building as static libraries), you may set `use_modular_headers!` globally in your Podfile, or specify `:modular_headers => true` for particular dependencies.
```

Indeed, this pods were added as dependencies in `packages/react-native/scripts/cocoapods/new_architecture.rb` but do not define modules contrary to the other pods in the list.

This PR is solving a problem that already occured in the past and was solved here: #33743
It's a new implementation for the PR initially opened here: #38039

## Changelog:
[IOS] [FIXED] - Fix pod install for libraries using Swift code when the new architecture is enabled

Pull Request resolved: #38121

Test Plan:
1. Clone [this](https://github.com/louiszawadzki/react-native) repo
2. From `main`, add a Swift file to the `MyNativeView` native module in the RN tester app (see inspiration from [this commit](fortmarek@26958fc))
3. Try to run `RCT_NEW_ARCH_ENABLED=1 USE_HERMES=0 bundle exec pod install` inside the `packages/rn-tester`
4. Observe errors
5. Apply [the commit](7b7c3ff) from this PR
6. Both pod install and the subsequent build should succeed.
7. Revert the changes and repeat steps 2 to 6 with `RCT_NEW_ARCH_ENABLED=1 USE_HERMES=1 bundle exec pod install`

Reviewed By: cortinico

Differential Revision: D47127854

Pulled By: cipolleschi

fbshipit-source-id: bf7f65e0d126195a76a0fafbe2f3172f00d5adc1
kelset pushed a commit that referenced this pull request Jul 10, 2023
Summary:
This fixes a bug that started with React Native 0.72.0 when using the new architecture and installing a native lib that has Swift code (in my case, `datadog/mobile-react-native`).

Running `pod install` errors with the following output (`DatadogSDKReactNative` is the pod containing the Swift code):

```
[...]
Analyzing dependencies
Downloading dependencies
Installing DatadogSDKReactNative 1.8.0-rc0
[!] The following Swift pods cannot yet be integrated as static libraries:

The Swift pod `DatadogSDKReactNative` depends upon `React-Fabric`, `React-graphics`, `React-utils`, and `React-debug`, which do not define modules. To opt into those targets generating module maps (which is necessary to import them from Swift when building as static libraries), you may set `use_modular_headers!` globally in your Podfile, or specify `:modular_headers => true` for particular dependencies.
```

Indeed, this pods were added as dependencies in `packages/react-native/scripts/cocoapods/new_architecture.rb` but do not define modules contrary to the other pods in the list.

This PR is solving a problem that already occured in the past and was solved here: #33743
It's a new implementation for the PR initially opened here: #38039

## Changelog:
[IOS] [FIXED] - Fix pod install for libraries using Swift code when the new architecture is enabled

Pull Request resolved: #38121

Test Plan:
1. Clone [this](https://github.com/louiszawadzki/react-native) repo
2. From `main`, add a Swift file to the `MyNativeView` native module in the RN tester app (see inspiration from [this commit](fortmarek@26958fc))
3. Try to run `RCT_NEW_ARCH_ENABLED=1 USE_HERMES=0 bundle exec pod install` inside the `packages/rn-tester`
4. Observe errors
5. Apply [the commit](7b7c3ff) from this PR
6. Both pod install and the subsequent build should succeed.
7. Revert the changes and repeat steps 2 to 6 with `RCT_NEW_ARCH_ENABLED=1 USE_HERMES=1 bundle exec pod install`

Reviewed By: cortinico

Differential Revision: D47127854

Pulled By: cipolleschi

fbshipit-source-id: bf7f65e0d126195a76a0fafbe2f3172f00d5adc1

# Conflicts:
#	packages/react-native/ReactCommon/react/renderer/debug/React-rendererdebug.podspec
Kudo pushed a commit to expo/react-native that referenced this pull request Jul 11, 2023
Summary:
This fixes a bug that started with React Native 0.72.0 when using the new architecture and installing a native lib that has Swift code (in my case, `datadog/mobile-react-native`).

Running `pod install` errors with the following output (`DatadogSDKReactNative` is the pod containing the Swift code):

```
[...]
Analyzing dependencies
Downloading dependencies
Installing DatadogSDKReactNative 1.8.0-rc0
[!] The following Swift pods cannot yet be integrated as static libraries:

The Swift pod `DatadogSDKReactNative` depends upon `React-Fabric`, `React-graphics`, `React-utils`, and `React-debug`, which do not define modules. To opt into those targets generating module maps (which is necessary to import them from Swift when building as static libraries), you may set `use_modular_headers!` globally in your Podfile, or specify `:modular_headers => true` for particular dependencies.
```

Indeed, this pods were added as dependencies in `packages/react-native/scripts/cocoapods/new_architecture.rb` but do not define modules contrary to the other pods in the list.

This PR is solving a problem that already occured in the past and was solved here: facebook#33743
It's a new implementation for the PR initially opened here: facebook#38039

## Changelog:
[IOS] [FIXED] - Fix pod install for libraries using Swift code when the new architecture is enabled

Pull Request resolved: facebook#38121

Test Plan:
1. Clone [this](https://github.com/louiszawadzki/react-native) repo
2. From `main`, add a Swift file to the `MyNativeView` native module in the RN tester app (see inspiration from [this commit](fortmarek@26958fc))
3. Try to run `RCT_NEW_ARCH_ENABLED=1 USE_HERMES=0 bundle exec pod install` inside the `packages/rn-tester`
4. Observe errors
5. Apply [the commit](facebook@7b7c3ff) from this PR
6. Both pod install and the subsequent build should succeed.
7. Revert the changes and repeat steps 2 to 6 with `RCT_NEW_ARCH_ENABLED=1 USE_HERMES=1 bundle exec pod install`

Reviewed By: cortinico

Differential Revision: D47127854

Pulled By: cipolleschi

fbshipit-source-id: bf7f65e0d126195a76a0fafbe2f3172f00d5adc1

# Conflicts:
#	packages/react-native/ReactCommon/react/renderer/debug/React-rendererdebug.podspec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Shopify Partner: Shopify Partner Platform: iOS iOS applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants