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

feat: convert project to be expo managed #300

Merged
merged 41 commits into from
May 7, 2024
Merged

feat: convert project to be expo managed #300

merged 41 commits into from
May 7, 2024

Conversation

CDFN
Copy link
Collaborator

@CDFN CDFN commented Apr 29, 2024

This PR converts project to be expo managed meaning we don't include android/ and ios/ folders anymore.
It also adds configuration for Expo services.

Closes #225
Closes #268 (now npm run android bundles and installs app to device and runs server via expo)

@CDFN CDFN marked this pull request as ready for review April 30, 2024 13:37
@CDFN CDFN requested review from achou11 and ErikSin April 30, 2024 13:37
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

Ran out of time today so not an exhaustive review - looking forward to testing this out though!

Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

Having issues getting this to run on my device, but that could be an issue on my end

Copy link
Member

Choose a reason for hiding this comment

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

a little surprised that this change is necessary. the source of the data is a static json file that should be inlined into the bundle (due to TS + Metro). that shouldn't change once the app starts, to my knowledge 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know, I was very suprised too it doesn't work. However for some reason the data would be undefined whenever I tried to debug this code as it caused errors. It is called once only so I guess should be fine to leave it as it is? Didn't encounter any issues in other places in project so I think it's just some weird edge case.

package.json Outdated
"start": "react-native start",
"android-no-backend-rebuild": "expo run:android",
"ios": "expo run:ios && npm run android-no-backend-rebuild",
"prestart": "npm run build:translations && npm run build:intl-polyfills && npm run build:backend",
Copy link
Member

Choose a reason for hiding this comment

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

i don't think building the backend should be part of prestart. ideally prestart only handles things that are necessary for start, which is predominantly frontend stuff.

if you do:

npm start
npm run android

you run the backend build step twice, which is not ideal

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, that seems fair. Currently it'll look like that:

  • postinstall triggers rebuild of all autogenerated files (translations, polyfills, backend),
  • prestart triggers generation of frontend stuff only (translations, polyfills),
  • android triggers rebuild of frontend + backend + enables android app,
  • ios triggers rebuild of frontend + backend + enables ios app,
  • android-no-backend-rebuild, as name says, just rebuilds frontend and enables android app,
  • ios-no-backend-rebuild is analogous to android task,
  • build:autogenerated is just alias for generating translations, polyfills, backend

LMK if we should add some setup notes to readme, as this autogenerated setup is a little bit tricky to understand at first glance.

Copy link
Member

Choose a reason for hiding this comment

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

more of a personal preference, but not entirely sure if we need postinstall to do all of the work related to autogenerated files. at the moment, the ones in question are only needed for when you want to run the app (or tests once we have more of them) so i'd think that doing that work in prestart and before android would be okay. i like having npm install being as fast as possible, with it mostly being used to deal with setting up the dev environment (not necessarily app code). again, not adamant about this as it's just my personal preference.

otherwise your breakdown seems reasonable to me!

LMK if we should add some setup notes to readme, as this autogenerated setup is a little bit tricky to understand at first glance.

Think it's okay to not document this for now since things are still changing a bit. The long-term plan is to have a contributors/development-specific doc that details this stuff, which I think would be easier for someone like myself to do :)

Copy link
Member

@achou11 achou11 May 7, 2024

Choose a reason for hiding this comment

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

after some more thinking, think we should remove the build:autogenerated script and remove its usage from postinstall. think it will make sense to revisit all the scripts setup in a separate PR, but introducing it here unnecessarily increases CI time (more specifically running the build:backend command when it's not needed for the CI tasks)

EDIT: done in c17bdc6

@achou11
Copy link
Member

achou11 commented May 1, 2024

Adding here to track: I had to pass the --localhost flag for expo start (default is host) in order to get the app running on a physical device. this seems like an Expo bug but haven't found anything reported just yet. Ideally this should be working on LAN 🤔

EDIT: Things are working for me now without any changes. wonder what happened...

@digidem digidem deleted a comment from expo bot May 6, 2024
@CDFN CDFN requested a review from achou11 May 6, 2024 11:32
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

Noticed that the input elements are different due to the prebuild generating some base styles at the native level in android/app/src/main/res/values/styles.xml (see screenshots below):

<resources xmlns:tools="http://schemas.android.com/tools">
  <style name="AppTheme" parent="Theme.AppCompat.Light.NoActionBar">
    <item name="android:textColor">@android:color/black</item>
    <item name="android:editTextStyle">@style/ResetEditText</item>
    <item name="android:editTextBackground">@drawable/rn_edit_text_material</item>
    <item name="colorPrimary">@color/colorPrimary</item>
    <item name="colorPrimaryDark">@color/colorPrimaryDark</item>
  </style>
  <style name="ResetEditText" parent="@android:style/Widget.EditText">
    <item name="android:padding">0dp</item>
    <item name="android:textColorHint">#c8c8c8</item>
    <item name="android:textColor">@android:color/black</item>
  </style>
  <style name="Theme.App.SplashScreen" parent="AppTheme">
    <item name="android:windowBackground">@drawable/splashscreen</item>
  </style>
</resources>

Not entirely against this happening since it kind of acts like a style reset but it does mean that we need to go through the app to see what needs adjusting. Luckily there aren't too many places with inputs, I think...

More generally, do you know where the docs for configuring this are?


On my Pixel 6, Android 14...

Before:

image

After:

image

@bohdanprog
Copy link
Collaborator

bohdanprog commented May 7, 2024

@achou11 here is link

Noticed that the input elements are different due to the prebuild generating some base styles at the native level in android/app/src/main/res/values/styles.xml (see screenshots below):

<resources xmlns:tools="http://schemas.android.com/tools">
  <style name="AppTheme" parent="Theme.AppCompat.Light.NoActionBar">
    <item name="android:textColor">@android:color/black</item>
    <item name="android:editTextStyle">@style/ResetEditText</item>
    <item name="android:editTextBackground">@drawable/rn_edit_text_material</item>
    <item name="colorPrimary">@color/colorPrimary</item>
    <item name="colorPrimaryDark">@color/colorPrimaryDark</item>
  </style>
  <style name="ResetEditText" parent="@android:style/Widget.EditText">
    <item name="android:padding">0dp</item>
    <item name="android:textColorHint">#c8c8c8</item>
    <item name="android:textColor">@android:color/black</item>
  </style>
  <style name="Theme.App.SplashScreen" parent="AppTheme">
    <item name="android:windowBackground">@drawable/splashscreen</item>
  </style>
</resources>

Not entirely against this happening since it kind of acts like a style reset but it does mean that we need to go through the app to see what needs adjusting. Luckily there aren't too many places with inputs, I think...

More generally, do you know where the docs for configuring this are?

On my Pixel 6, Android 14...

Before:

image After: image

Hey, right, we missed that, I added an expo-config-plugin that removed those styles.

@achou11
Copy link
Member

achou11 commented May 7, 2024

@CDFN @bohdanprog heads up that i cleaned up the custom expo config plugin implementation in 14be5c8. figured it would be easier to do it myself than have more back and forth since it was the last thing i wanted to address before approving the PR :)

Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

Overall changes are now working well, terrific work! 👍 Any smaller issues are/can be follow-up PRs

@achou11 achou11 merged commit ecbc8d5 into main May 7, 2024
3 checks passed
@achou11 achou11 deleted the feat/expo-prebuild branch May 7, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

npm run android shouldn't require npm start to run Adopt Expo Prebuild
3 participants