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 building error in expo project #1249

Merged
merged 1 commit into from
Jan 20, 2023
Merged

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Jan 21, 2022

Why

in expo sdk 44+ project, we opened formal swift integration. for swift to call objective-c functions, sometimes the #import format should be done right. learn more from expo/expo#15622 (comment)

fix #1237
fix expo/expo#15986

How

watermelonDB has a call from DatabaseBridge.swift to JSIInstaller.h. for swift to call objective-c functions, we should make JSIInstaller.h as a clang module. this pr moves JSIInstaller.h as cocoapods public header and the generated umbrella header will include JSIInstaller.h as a clang submodule.

Test Plan

expo sdk 44 project

  1. expo init sdk44 # select bare
  2. yarn add @nozbe/watermelondb
  3. patch watermelondb with the pr changes
  4. add these to ios/Podfile
  pod 'React-jsi', :path => '../node_modules/react-native/ReactCommon/jsi', :modular_headers => true
  pod 'simdjson', path: '../node_modules/@nozbe/simdjson'
  1. npx pod-install
  2. yarn ios

react-native 0.66 project

  1. npx react-native init RN066 --version 0.66
  2. yarn add @nozbe/watermelondb
  3. patch watermelondb with the pr changes
  4. add these to ios/Podfile
  pod 'React-jsi', :path => '../node_modules/react-native/ReactCommon/jsi', :modular_headers => true
  pod 'simdjson', path: '../node_modules/@nozbe/simdjson'
  1. npx pod-install
  2. yarn ios

@enahum
Copy link

enahum commented Feb 26, 2022

@radex thoughts on having this merged ?

@aircliff01
Copy link

Hey All, any reason this PR hasnt been merged? WatermelonDB is broken and pretty unusable on expo 44. This PR fixes the issue.

@thecoorum
Copy link

@Kudo does this work for expo projects with dev-client and expo prebuild -p ios? After following instructions it's failing on EAS Fastlane step with:

Compiling @nozbe/watermelondb Pods/WatermelonDB » Database.swift

❌  (node_modules/@nozbe/watermelondb/native/ios/WatermelonDB/DatabaseBridge.swift:93:13)

  91 |         methodQueue.sync {
  92 |             // swiftlint:disable all
> 93 |             installWatermelonJSI(bridge as? RCTCxxBridge)
     |             ^ cannot find 'installWatermelonJSI' in scope
  94 |         }
  95 |         return [:]
  96 |     }

❌  (node_modules/@nozbe/watermelondb/native/ios/WatermelonDB/DatabaseBridge.swift:103:9)

  101 |                          resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) {
  102 |         var error: NSError?
> 103 |         watermelondbProvideSyncJson(id.int32Value, json.data(using: String.Encoding.utf8.rawValue), &error)
      |         ^ cannot find 'watermelondbProvideSyncJson' in scope
  104 |         if let error = error {
  105 |             sendReject(reject, error)
  106 |         } else {

⚠️  (../../Library/Developer/Xcode/DerivedData/track-ezmllxjvzftvvzcdtnxcfnrldxbo/Build/Intermediates.noindex/ArchiveIntermediates/track/IntermediateBuildFilesPath/Pods.build/Debug-iphoneos/WatermelonDB.build/Objects-normal/arm64/Database-f1a153e176371a1f77b80228b6a743e6.dia:1:1)

> 1 | 
    | ^ Could not read serialized diagnostics file: error(in target 'WatermelonDB' from project 'Pods')
› Compiling @nozbe/watermelondb Pods/WatermelonDB » DatabaseBridge.swift

❌  (node_modules/@nozbe/watermelondb/native/ios/WatermelonDB/DatabaseBridge.swift:93:13)

  91 |         methodQueue.sync {
  92 |             // swiftlint:disable all
> 93 |             installWatermelonJSI(bridge as? RCTCxxBridge)
     |             ^ cannot find 'installWatermelonJSI' in scope
  94 |         }
  95 |         return [:]
  96 |     }

❌  (node_modules/@nozbe/watermelondb/native/ios/WatermelonDB/DatabaseBridge.swift:103:9)

  101 |                          resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) {
  102 |         var error: NSError?
> 103 |         watermelondbProvideSyncJson(id.int32Value, json.data(using: String.Encoding.utf8.rawValue), &error)
      |         ^ cannot find 'watermelondbProvideSyncJson' in scope
  104 |         if let error = error {
  105 |             sendReject(reject, error)
  106 |         } else {

and

The following build commands failed:
▸ 	CompileSwift normal arm64 /Users/expo/workingdir/build/node_modules/@nozbe/watermelondb/native/ios/WatermelonDB/Database.swift (in target 'WatermelonDB' from project 'Pods')
▸ 	CompileSwift normal arm64 /Users/expo/workingdir/build/node_modules/@nozbe/watermelondb/native/ios/WatermelonDB/DatabaseBridge.swift (in target 'WatermelonDB' from project 'Pods')
▸ 	CompileSwiftSources normal arm64 com.apple.xcode.tools.swift.compiler (in target 'WatermelonDB' from project 'Pods')
▸ 	CompileSwiftSources normal arm64 com.apple.xcode.tools.swift.compiler (in target 'expo-dev-menu' from project 'Pods')
▸ (4 failures)

@Kudo
Copy link
Contributor Author

Kudo commented Mar 21, 2022

@thecoorum do you have some example repo? because the installation requires add extra pods to Podfile, i am assuming you would have a bare workflow app. otherwise, a config-plugin to add pods is required.
i've verified with this steps and it works for me.

  1. expo init sdk44 # select bare
  2. expo install expo-dev-client
  3. yarn add @nozbe/watermelondb
  4. patch watermelondb with the pr changes
  5. add these to ios/Podfile
  pod 'React-jsi', :path => '../node_modules/react-native/ReactCommon/jsi', :modular_headers => true
  pod 'simdjson', path: '../node_modules/@nozbe/simdjson'
  1. npx pod-install
  2. yarn ios

@KrisLau
Copy link
Contributor

KrisLau commented Apr 19, 2022

@radex Waiting on this to be merged as migrating to expo Modules (specifically Expo 44) from unimodules breaks WatermelonDB

Copy link

@ruifernandees ruifernandees left a comment

Choose a reason for hiding this comment

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

I've tested and it is working good.

Copy link

@vtpa vtpa left a comment

Choose a reason for hiding this comment

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

Approved!

@zsinryu
Copy link

zsinryu commented May 1, 2022

@vtpa @Kudo this have been approved.
Can you guys please merge this?

@jwoo92
Copy link

jwoo92 commented May 1, 2022

I was able to get @nozbe/watermelondb@0.24.0 to work with Expo 44 after applying this patch.

Thank you 👍

@KrisLau
Copy link
Contributor

KrisLau commented May 5, 2022

@vtpa @Kudo this have been approved. Can you guys please merge this?

@zsinryu They cannot merge the pull request since I believe only @radex has that permission

@zsinryu
Copy link

zsinryu commented May 5, 2022

@radex can you check this?

@Hannes1
Copy link

Hannes1 commented May 5, 2022

Hey guys, can someone please post their patch package diff file, mine keeps failing when I generate it😭

@jwoo92
Copy link

jwoo92 commented May 5, 2022

Hey guys, can someone please post their patch package diff file, mine keeps failing when I generate it😭

#1237 (comment)

@Hannes1
Copy link

Hannes1 commented May 5, 2022

Hey guys, can someone please post their patch package diff file, mine keeps failing when I generate it😭

#1237 (comment)

Thanks @jwoo92 , I tried this already and keep getting a error when running npx patch, this is why I'm asking for the file someone already generated in their patch folder

@jwoo92
Copy link

jwoo92 commented May 5, 2022

@Hannes1 Oh I missed that, here you go:

patches/@nozbe+watermelondb+0.24.0.patch

diff --git a/node_modules/@nozbe/watermelondb/WatermelonDB.podspec b/node_modules/@nozbe/watermelondb/WatermelonDB.podspec
index 1f3af50..e9fea58 100644
--- a/node_modules/@nozbe/watermelondb/WatermelonDB.podspec
+++ b/node_modules/@nozbe/watermelondb/WatermelonDB.podspec
@@ -13,7 +13,10 @@ Pod::Spec.new do |s|
   s.platforms    = { :ios => "9.0", :tvos => "9.0" }
   s.source = { :git => "https://github.com/Nozbe/WatermelonDB.git", :tag => "v#{s.version}" }
   s.source_files = "native/ios/**/*.{h,m,mm,swift,c,cpp}", "native/shared/**/*.{h,c,cpp}"
-  s.public_header_files = 'native/ios/WatermelonDB/SupportingFiles/Bridging.h'
+  s.public_header_files = [
+    'native/ios/WatermelonDB/SupportingFiles/Bridging.h',
+    'native/ios/WatermelonDB/JSIInstaller.h',
+  ]
   s.requires_arc = true
   # simdjson is annoyingly slow without compiler optimization, disable for debugging
   s.compiler_flags = '-Os'
diff --git a/node_modules/@nozbe/watermelondb/native/ios/WatermelonDB/SupportingFiles/Bridging.h b/node_modules/@nozbe/watermelondb/native/ios/WatermelonDB/SupportingFiles/Bridging.h
index 135118d..8324550 100644
--- a/node_modules/@nozbe/watermelondb/native/ios/WatermelonDB/SupportingFiles/Bridging.h
+++ b/node_modules/@nozbe/watermelondb/native/ios/WatermelonDB/SupportingFiles/Bridging.h
@@ -6,12 +6,6 @@
 
 #import <React/RCTBridgeModule.h>
 
-#if __has_include("JSIInstaller.h")
-#import "JSIInstaller.h"
-#else
-#import "../JSIInstaller.h"
-#endif
-
 #if __has_include("DatabaseDeleteHelper.h")
 #import "DatabaseDeleteHelper.h"
 #else

@Hannes1
Copy link

Hannes1 commented May 5, 2022

@jwoo92 Greatly appreciated, thanks for the help🙏

Copy link

@diegoalvescf diegoalvescf left a comment

Choose a reason for hiding this comment

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

Approved, I want this PR until tomorrow merged

Copy link

@jamesjlv jamesjlv left a comment

Choose a reason for hiding this comment

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

Approved!

@zsinryu
Copy link

zsinryu commented May 22, 2022

@radex why this is not merged?

@alexyoungs
Copy link

Hey guys, checking in on whether this can be merged?

@KrisLau
Copy link
Contributor

KrisLau commented Jun 13, 2022

@alexyoungs Read the above thread. None of us have permission to do so, so it's currently just awaiting review/approval from @radex

@ansh
Copy link

ansh commented Sep 4, 2022

Why is this PR not merged? It seems like it works.

Pining @radex and @RafalNozbe for help

@itzzritik
Copy link

@radex This PR needs to be merged, as WatermelonDB is no longer working for EXPO 44+
Looks like it's working for most people with this fix.

Can we merge it?

@Dallas62
Copy link

Same fix but with:

  s.public_header_files = [
      'native/ios/WatermelonDB/**/*.h',
  ]

to build the library on 0.70.x and use_frameworks! :linkage => :static

@ansh
Copy link

ansh commented Dec 20, 2022

@radex Some communication is the bare minimum after months of a valid PR from the Expo team.

@radex radex merged commit 78d9562 into Nozbe:master Jan 20, 2023
@radex
Copy link
Collaborator

radex commented Jan 20, 2023

@Kudo Thank you

@Dallas62
Copy link

Hi @radex, I still had to change the content of WatermelonDB.podspec with:

  s.public_header_files = [
      'native/ios/WatermelonDB/**/*.h',
  ]

And copy :

node_modules/@nozbe/simdjson/src/simdjson.h

In :

node_modules/@nozbe/watermelondb/native/shared

On 0.70.6

@radex
Copy link
Collaborator

radex commented Jan 23, 2023

@Dallas62 Can you please elaborate? What issues did you see, what changes did you have to make, which Expo version?

@Dallas62
Copy link

This issue doesn't happen with Expo, it's mainly not resolve import in the native side (same file of this issue).

Will provide more details later

@radex
Copy link
Collaborator

radex commented Jan 23, 2023

@Dallas62 Please do provide more details. Are you using Turbo Sync by any chance?

@KrisLau
Copy link
Contributor

KrisLau commented Jan 23, 2023

@radex so the issue @Dallas62 was facing is probably related to use_frameworks! and I also have the same thing patched. It would be nice to be able to have it permanently fixed since react-native-firebase requires the use of use_frameworks! now.

Relevant thread: #1285
My changes: #1285 (comment)

@Dallas62
Copy link

@KrisLau Exactly this case, all details are mentioned in this comment #1285 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet