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

[core][iOS] Clean up ExpoModulesCore.podspec #31044

Merged
merged 4 commits into from
Aug 21, 2024

Conversation

tsapeta
Copy link
Member

@tsapeta tsapeta commented Aug 18, 2024

Why

The podspec got a bit messy over time. Now we can include Fabric-specific code as Fabric pods are installed by default anyway.

How

  • Moved requiring RN scripts to the top (pod ipc spec was failing because we're requiring it after we call add_dependency)
  • Cleaned up compiler flags and xcconfigs
  • Removed using vendored frameworks (we don't prebuild the modules core)
  • Added React-RCTFabric dependency even if Fabric is not enabled
  • Always include ios/Fabric and common/fabric folders
  • Replaced the legacy RN_FABRIC_ENABLED with RCT_NEW_ARCH_ENABLED

Test Plan

  • bare-expo builds and runs just fine
  • iOS Client on EAS Build check is failing, but it's unrelated (and also broken on main)

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Aug 18, 2024
@expo-bot
Copy link
Collaborator

expo-bot commented Aug 18, 2024

The Pull Request introduced fingerprint changes against the base commit: df90d60

Fingerprint diff
[
  {
    "op": "changed",
    "source": {
      "type": "dir",
      "filePath": "../../packages/expo-modules-core",
      "reasons": [
        "expoAutolinkingIos",
        "expoAutolinkingAndroid"
      ],
      "hash": "5c83c424bda454efb2cebcb5c21dccde1421d079"
    }
  },
  {
    "op": "changed",
    "source": {
      "type": "dir",
      "filePath": "ios",
      "reasons": [
        "bareNativeDir"
      ],
      "hash": "d6c2df3f5627709a916dc8bfac3acc837a857ffc"
    }
  }
]

Generated by PR labeler 🤖

@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Aug 18, 2024
@tsapeta tsapeta changed the title [core] Clean up ExpoModulesCore.podspec [core][iOS] Clean up ExpoModulesCore.podspec Aug 18, 2024
@tsapeta tsapeta marked this pull request as ready for review August 19, 2024 08:41
@tsapeta tsapeta requested review from Kudo and lukmccall as code owners August 19, 2024 08:41
Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

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

much appreciated for the change. leaving some comments for possibly improvements

packages/expo-modules-core/ExpoModulesCore.podspec Outdated Show resolved Hide resolved
"HEADER_SEARCH_PATHS" => user_header_search_paths,
"HEADER_SEARCH_PATHS" => [
'"${PODS_CONFIGURATION_BUILD_DIR}/ExpoModulesCore/Swift Compatibility Header"',
'"$(PODS_ROOT)/Headers/Private/Yoga"', # Expo.h -> ExpoModulesCore-umbrella.h -> Fabric ViewProps.h -> Private Yoga headers
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'"$(PODS_ROOT)/Headers/Private/Yoga"', # Expo.h -> ExpoModulesCore-umbrella.h -> Fabric ViewProps.h -> Private Yoga headers

maybe we don't need Yoga because react-native core will add Yoga for modules

Copy link
Member Author

@tsapeta tsapeta Aug 20, 2024

Choose a reason for hiding this comment

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

Sadly, it fails on compiling AppDelegate.mm – it can't find <yoga/style/Style.h> 🤔 I'll investigate this later and for now I would just go ahead and merge it as is.

packages/expo-modules-core/ExpoModulesCore.podspec Outdated Show resolved Hide resolved
@tsapeta tsapeta force-pushed the @tsapeta/clean-up-emc-podspec branch from 798d2f9 to bbc0308 Compare August 20, 2024 22:00
@tsapeta tsapeta merged commit a8d0772 into main Aug 21, 2024
12 of 13 checks passed
@tsapeta tsapeta deleted the @tsapeta/clean-up-emc-podspec branch August 21, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint changed bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants