-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Conversation
Base commit: e7d9e4d |
Base commit: e7d9e4d |
Hi @fortmarek, thank you for the PR. The code looks fine to me, I will call the module 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:
Additionally, as highlighted here, we can drop the |
9772c62
to
214ed81
Compare
Thanks for the review @cipolleschi 🙌 I have updated the module name to
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.
Right, I definitely can, the example was maybe put together too hastily as it was not supposed to be merged 🙂 thanks for the suggestion 👍
On
Thanks, I was not aware of that! Still new to the Fabric arch ^^ |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@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 |
214ed81
to
50674ac
Compare
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 |
All good, I agree with the changes. I will reimport the PR! |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @fortmarek in 709a459. When will my fix make it into a release? | Upcoming Releases |
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
Hey @fortmarek! Thanks for this and the upcoming 0.69 release.
Never mind, I had a typo on my import 🤦🏻 Cheers. |
Glad that you found out the issue @andrestone 🙌 |
@fortmarek Will this be part of RN 0.69 release? |
Yes, you can find all the picks for 0.69 here |
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. |
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
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
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
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:To resolve this, I have applied the suggestion from the error (set
:modular_headers => true
for the appropriate modules insidereact_native_pods.rb
.Afterwards,
pod install
succeeds but I still gotRedefinition of module 'React'
during the build due to the conflict inside the generated modulesmapsReact-Core.modulemap
andReact-RCTFabric.modulemap
. This makes sense sinceReact-RCTFabric.podspec
hass.header_dir = "React"
(see here) and the module inherits that. However, we can explicitly specifymodule_name
for the podspec which is what I have done. I have named the moduleFabric
, 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
main
, apply changes from this commit (adding Swift file to theMyNativeView
native module in the RN tester app)USE_FABRIC=1 RCT_NEW_ARCH_ENABLED=1 USE_CODEGEN_DISCOVERY=1 USE_HERMES=0 bundle exec pod install
inside thepackages/rn-tester
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 onmain