Skip to content

feat: add expo modules to the project #1524

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

MosCD3
Copy link
Contributor

@MosCD3 MosCD3 commented May 12, 2025

Summary of Changes

  • Enabling Bifold sample app to bundle expo modules core to be able to run expo packages
  • Adding required packages to the core

Screenshots, videos, or gifs

Replace this text with embedded media for UI changes if they are included in this PR. If there are none, simply enter N/A

Breaking change guide

Replace this text with any breaking changes included in this PR along with how to address them in downstream projects. If there are none, simply enter N/A

Related Issues

Replace this text with issue #'s that are relevant to this PR. If there are none, simply enter N/A

Pull Request Checklist

Tick all boxes below to demonstrate that you have completed the respective task. If the item does not apply to your this PR check it anyway to make it apparent that there's nothing to do.

  • All commits contain a DCO Signed-off-by line (we use the DCO GitHub app to enforce this)
  • If applicable, screenshots, gifs, or video are included for UI changes
  • If applicable, breaking changes are described above along with how to address them
  • If applicable, added changeset(s)
  • Added sufficient tests so that overall code coverage is not reduced

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

Pro Tip 🤓

  • Read our contribution guide at least once; it will save you a few review cycles!
  • Your PR will likely not be reviewed until all the above boxes are checked and all automated checks have passed

Signed-off-by: Mostafa Gamal <[email protected]>
Copy link

changeset-bot bot commented May 12, 2025

🦋 Changeset detected

Latest commit: b33511c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@bifold/core Minor
@bifold/remote-logs Minor
@bifold/oca Minor
@bifold/react-native-attestation Minor
@bifold/verifier Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jleach jleach changed the title Add expo modules to the project feat: add expo modules to the project May 13, 2025
@MosCD3 MosCD3 marked this pull request as ready for review May 29, 2025 18:11
@MosCD3 MosCD3 requested a review from a team as a code owner May 29, 2025 18:11
Copy link

Comment on lines +58 to +59
"@unimodules/react-native-adapter": "file:./noop",
"@unimodules/core": "file:./noop",
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is an issue with credo it uses unsupported package tat you must exclude
issue described here
https://credo.js.org/guides/getting-started/set-up

@@ -473,7 +545,7 @@
"$(inherited)",
);
INFOPLIST_FILE = AriesBifoldTests/Info.plist;
IPHONEOS_DEPLOYMENT_TARGET = 12.0;
IPHONEOS_DEPLOYMENT_TARGET = 13.4;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we avoid this bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I came across this and expo failed to build until I bump up the version

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should at least try to keep the latest 12.x version supported, it's a non-negligible percentage of iOS users

Copy link
Contributor

Choose a reason for hiding this comment

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

Researched this a bit, seems like it's pretty difficult to support lower versions than the default lowest Expo comes with... I'll bring it up to the BC Team

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a look at that

[20:44:40]: Application Loader output above ^
[20:44:40]: ERROR: [ContentDelivery.Uploader] Validation failed (409) SDK version issue. This app was built with the iOS 17.5 SDK. All iOS and iPadOS apps must be built with the iOS 18 SDK or later, included in Xcode 16 or later, in order to be uploaded to App Store Connect or submitted for distribution. (ID: a7efe6f5-62e0-42cc-be04-2cdbadd22e95)

Now apple requires minimum target version to be 1OS 18.

Copy link
Contributor

Choose a reason for hiding this comment

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

just thought to leave a comment :D about this it is about the built SDK not the minimum IOS deployment version so it is just about which Xcode version you are using it should be 16+ and you can find the SDK version beside simulator and it will be shown in the metadata when you submit that is why it would be rejected on app store @MosCD3

Copy link
Contributor

Choose a reason for hiding this comment

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

as for why you need to bump it up it is mentioned in expo SDK 50 changelog Expo 50

snapshot of documentation
tldr : the iOS minimum deployment target should be bumped to 13.4

@@ -51,6 +51,8 @@
"@react-navigation/stack": "^6.3.21",
"axios": "^1.4.0",
"base-64": "^1.0.0",
"expo": "~50.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

patch version could be 50.0.20, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you think this will work, can give it a shot but you will be responsible for wwIII

@bryce-mcmath bryce-mcmath added the do not merge Don't merge when label present label Jun 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Don't merge when label present
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants