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

Upgrade to newest RN, add Expo SDK, general cleanup. #550

Merged
merged 34 commits into from
Mar 6, 2024

Conversation

vonovak
Copy link
Contributor

@vonovak vonovak commented Jan 27, 2024

this PR does the following:

  • it adds expo SDK 50 (and react native 0.73) to the project and aims to use the Development build approach so that we can use the benefits of Expo such as easy upgrades, while still be able to use custom native code.
  • it removes a bunch of old libraries such as rn-nodeify, react-native-camera and more, and replaces those with better alternatives
  • it upgrades some major dependencies (e.g. realm) because after upgrading to RN 0.73 (the latest stable RN version), some packages simply stopped being compatible
  • removes the ios and android folders because we should be able to generate them using expo prebuild
  • when you start the project, you'll see that metro complains about many (!) circular dependencies. I tried to remove some because I was running into an issue and believed circular deps were the problem (they weren't). So, these changes are not directly related to this PR and I can revert them if that's preferred, because they are making this PR pretty big
  • this is still WIP. The app builds and runs on both platforms for me (though on Android I needed to remove the flipper dependency in app/build.gradle) but needs to be tested thoroughly. I suspect that especially stuff related to deep links / adding credentials from outside the app will be broken.

If anyone could provide reproduction steps for issues, that would be highly appreciated, as I'm not very familiar with the app yet. To run the app, you need to check out this branch, install dependencies, run prebuild and then carry on as usual :)

@dmitrizagidulin dmitrizagidulin changed the title feat: add expo Upgrade to newest RN, add Expo SDK, general cleanup. Jan 27, 2024
@dmitrizagidulin
Copy link
Contributor

@vonovak Thank you so much! We'll make sure to test it, and add any issues with repro steps in comments here.

@dmitrizagidulin
Copy link
Contributor

@vonovak Sounds like another TODO item is - update the github CI android & ios build actions, to use the new Development Build method.

@alexfigtree
Copy link
Contributor

I did a fresh clone, yarn install (no --legacy-peer-deps flag), then yarn start, and after installing on my Android device using Expo, I see the following:

Screenshot_20240205-113430
Screenshot_20240205-113439

@vonovak
Copy link
Contributor Author

vonovak commented Feb 5, 2024

Hey! It's possible I messed something up as I was pushing today but:

  • Please use npm to install the deps. That way the lock file is being used
  • please don't forget to run the prebuild scripts (run android and then ios)

I'm afk now but will get to this layer today again

@dmitrizagidulin
Copy link
Contributor

@vonovak Thanks! Do you mind updating the README, with the exact steps to launch? (I'm not sure what the prebuild scripts entail)

@vonovak
Copy link
Contributor Author

vonovak commented Feb 5, 2024

Will do. There are two prebuild scripts, one for each platform. They are in package.json

@alexfigtree
Copy link
Contributor

I ran the prebuild for android and came across this error:
Screenshot 2024-02-05 at 2 30 59 PM

Tried running expo again but came across the same errors as before:
Screenshot 2024-02-05 at 2 32 19 PM

@vonovak
Copy link
Contributor Author

vonovak commented Feb 5, 2024

@alexfigtree thank you for testing things out.

Firstly, please share your error information as text. Images are hard to work with mainly because it's harder to copy content from them and also because they are not searchable - if someone searches for some text in your screenshot, github won't find it.

I have pushed a change that fixes an android build issue, but it's unrelated to what you're seeing:

Looking at the first screenshot: you're using yarn, as seen in the log. Please use npm because that way you'll get the same dependencies installed as me and everyone else. Make sure that you don't have yarn.lock in the root, and generally make sure to use clean git state at da2959e

Secondly, I see "using current versions instead of the recommended...." - that's suspicious, such warning should not be present. Here's my output of the command:

/opt/homebrew/bin/npm run prebuild:android

> learner-credential-wallet@1.0.0 prebuild:android
> EXPO_NO_TELEMETRY=1 EXPO_NO_GIT_STATUS=1 npx expo prebuild --clean -p android && ./disableAndroidFlipper.sh

Git status is dirty but the command will continue because EXPO_NO_GIT_STATUS is enabled...
(node:86135) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
✔ Cleared android code
✔ Created native directory
✔ Updated package.json | no changes
✔ Finished prebuild
++ sed -i '' '/implementation("com.facebook.react:flipper-integration")/s/^/\/\/ /' ./android/app/build.gradle
++ sed -i '' '/ReactNativeFlipper.initializeFlipper(this, reactNativeHost.reactInstanceManager)/s/^/\/\/ /' ./android/app/src/main/java/edu/wallet/MainApplication.kt
++ sed -i '' '/import com.facebook.react.flipper.ReactNativeFlipper/s/^/\/\/ /' ./android/app/src/main/java/edu/wallet/MainApplication.kt

if you're having troubles getting the same output, it might be best to have a call about it where we debug what's going on.

Hope this helps! :)

@alexfigtree alexfigtree linked an issue Feb 21, 2024 that may be closed by this pull request
@dmitrizagidulin
Copy link
Contributor

Testing latest on iOS. Everything seems to be working, but the following VC is showing up as 'Revoked', which is incorrect:

{
  "@context": [
    "https://www.w3.org/2018/credentials/v1",
    "https://w3id.org/security/suites/ed25519-2020/v1",
    "https://w3id.org/dcc/v1",
    "https://w3id.org/vc/status-list/2021/v1"
  ],
  "type": [
    "VerifiableCredential",
    "Assertion"
  ],
  "issuer": {
    "id": "did:key:z6MkhVTX9BF3NGYX6cc7jWpbNnR7cAjH8LUffabZP8Qu4ysC",
    "name": "Example University",
    "url": "https://cs.example.edu",
    "image": "https://user-images.githubusercontent.com/947005/133544904-29d6139d-2e7b-4fe2-b6e9-7d1022bb6a45.png"
  },
  "issuanceDate": "2020-08-16T12:00:00.000+00:00",
  "credentialSubject": {
    "id": "did:key:z6MkhVTX9BF3NGYX6cc7jWpbNnR7cAjH8LUffabZP8Qu4ysC",
    "name": "Kayode Ezike",
    "hasCredential": {
      "type": [
        "EducationalOccupationalCredential"
      ],
      "name": "GT Guide",
      "description": "The holder of this credential is qualified to lead new student orientations."
    }
  },
  "expirationDate": "2025-08-16T12:00:00.000+00:00",
  "credentialStatus": {
    "id": "https://digitalcredentials.github.io/credential-status-playground/JWZM3H8WKU#2",
    "type": "StatusList2021Entry",
    "statusPurpose": "revocation",
    "statusListIndex": 2,
    "statusListCredential": "https://digitalcredentials.github.io/credential-status-playground/JWZM3H8WKU"
  },
  "proof": {
    "type": "Ed25519Signature2020",
    "created": "2022-08-19T06:55:17Z",
    "verificationMethod": "did:key:z6MkhVTX9BF3NGYX6cc7jWpbNnR7cAjH8LUffabZP8Qu4ysC#z6MkhVTX9BF3NGYX6cc7jWpbNnR7cAjH8LUffabZP8Qu4ysC",
    "proofPurpose": "assertionMethod",
    "proofValue": "z4EiTbmC79r4dRaqLQZr2yxQASoMKneHVNHVaWh1xcDoPG2eTwYjKoYaku1Canb7a6Xp5fSogKJyEhkZCaqQ6Y5nw"
  }
}

(It's showing up as valid on https://verifierplus.org/ )

Investigating...

@dmitrizagidulin
Copy link
Contributor

Ok, narrowed down the error to an issue in @digitalcredentials/security-document-loader,
[Error: NotFoundError loading "https://digitalcredentials.github.io/credential-status-playground/JWZM3H8WKU": Cannot read property 'get' of undefined]

Which suggests something is going wrong with the http-client import.

@@ -5,16 +5,14 @@
"license": "BSD-3-Clause",
"type": "module",
"main": "./dist/cjs/index.cjs",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

metro loaded this implementation, which is for node. That was the issue

@@ -4,5 +4,8 @@ const { getDefaultConfig } = require('expo/metro-config');
const defaultConfig = getDefaultConfig(__dirname);

defaultConfig.resolver.assetExts.push('cjs');
// this will be the default in the future, so let's be future-proof
defaultConfig.resolver.enablePackageExports = true;
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 a major change in how metro resolves packages. It's not needed but it's going to be the default in future, so I figured I'd give it a try

@@ -30,6 +30,9 @@
}
}
},
"overrides": {
"@digitalbazaar/http-client": "4.1.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there were 2 versions of this installed, because @interop/did-web-resolver also depends on a different version of http-client

@alexfigtree
Copy link
Contributor

Seeing this after the android prebuild step, yarn start, then using Expo Go app:
Error: react-native-quick-crypto is not supported in Expo Go! Use EAS (expo prebuild) or eject to a bare workflow instead., js engine: hermes

@vonovak
Copy link
Contributor Author

vonovak commented Feb 26, 2024

Seeing this after the android prebuild step, yarn start, then using Expo Go app: Error: react-native-quick-crypto is not supported in Expo Go! Use EAS (expo prebuild) or eject to a bare workflow instead., js engine: hermes

please use the development build, not Expo Go. When you run yarn start you should receive instructions on how to do it (alternatively, use Google please). thank you :)

@vonovak
Copy link
Contributor Author

vonovak commented Feb 26, 2024

@alexfigtree I updated some scripts in package.json, you can try deleting ios and android directories and running yarn ios and yarn android. Please let me know if that helps :)

@kezike
Copy link
Contributor

kezike commented Feb 28, 2024

@vonovak I was able to build and run the app in Android. However, as you predicted, there are new issues when adding credentials from outside of the wallet. I can work toward debugging them, but here they are in the meantime:

  1. The app rebundles when control is returned from a browser-based credential claiming portal to the app. Here is a screenshot of this issue:
    Screenshot_20240228-093820_LearnerCredentialWallet-w_orig-h_570

  2. After the app rebundles, it fails around the same library that @alexfigtree reported about earlier. Here is the full stack trace:

 ERROR  Error: Failed to install react-native-quick-crypto: The native `QuickCrypto` Module could not be found.
* Make sure react-native-quick-crypto is correctly autolinked (run `npx react-native config` to verify)
* Make sure gradle is synced.
* Make sure you ran `expo prebuild`.
* Make sure you rebuilt the app.
 ERROR  Invariant Violation: "main" has not been registered. This can happen if:
* Metro (the local dev server) is run from the wrong folder. Check if Metro is running, stop it and restart it in the current project.
* A module failed to load due to an error and `AppRegistry.registerComponent` wasn't called.

@vonovak
Copy link
Contributor Author

vonovak commented Feb 28, 2024

@vonovak I was able to build and run the app in Android. However, as you predicted, there are new issues when adding credentials from outside of the wallet. I can work toward debugging them, but here they are in the meantime:

hello and thanks for reporting @kezike. I can help if necessary, but I'll need clear repro steps, thank you.

As for the react-native-quick-crypto error, I think that's a different error from the one reported previously. I'd say I'd follow the advice that the error message is giving.

@dmitrizagidulin
Copy link
Contributor

as you predicted, there are new issues when adding credentials from outside of the wallet

@kezike Thanks for testing this! Let's move that to a separate issue. (Meaning, adding credentials from outside is currently broken on main also, unrelated to the RN upgrade.)
So, we'll tackle it separately.

@dmitrizagidulin
Copy link
Contributor

dmitrizagidulin commented Feb 28, 2024

@kezike your (and all of ours) main goal at this point is just 1) Make sure the wallet builds and runs on iOS, and 2) Wallet builds and runs on Android.
Then we can merge this PR to main and proceed from there.

(I've tested it and it builds & VCs verify on iOS. I need to reinstall Android Studio tho, to make sure it works on android.)

@dmitrizagidulin
Copy link
Contributor

@vonovak @alexfigtree Any objection to changing the Running LCW instructions in the README from yarn start to npm start? (Since we're using npm install anyways, and we don't include yarn in the dependencies, and one can't assume devs have it globally installed.)

@alexfigtree
Copy link
Contributor

@dmitrizagidulin No objections from me, I prefer npm too. Also, I found this error while running on the dev server in ios:

ERROR ViewPropTypes will be removed from React Native, along with all other PropTypes. We recommend that you migrate away from PropTypes and switch to a type system like TypeScript. If you need to continue using ViewPropTypes, migrate to the 'deprecated-react-native-prop-types' package.

@alexfigtree
Copy link
Contributor

Still seeing the above error in Android as well, reported here: #575 I suggest merging this PR, but fixing #575 before deploying to any app stores.

@vonovak
Copy link
Contributor Author

vonovak commented Mar 6, 2024

That error likely comes from some 3rd party dep. Let's do the improvements step by step, in different PRs. If we were to put all the fixes in this PR it'd become bloated.

@dmitrizagidulin
Copy link
Contributor

Merging - huge thank you to @vonovak for doing the work, to @alexfigtree and others for testing!

@dmitrizagidulin dmitrizagidulin merged commit 5f38c29 into main Mar 6, 2024
3 checks passed
@dmitrizagidulin dmitrizagidulin deleted the feat/add-expo branch March 6, 2024 17:48
@fabrii
Copy link

fabrii commented Aug 29, 2024

Hi @vonovak.

We can see that you updated the library used for database encryption, and also changed a number there from 256 to 32. Does this means that previous generated wallets will stop working, and we have to reset them?

Thanks

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

Successfully merging this pull request may close these issues.

OWF: Add Expo / upgrade to latest RN (0.73.4) Error: crypto.subtle not found
5 participants