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: iOS build #2234

Merged
merged 8 commits into from
Mar 19, 2024
Merged

Feat: iOS build #2234

merged 8 commits into from
Mar 19, 2024

Conversation

jfmcquade
Copy link
Collaborator

@jfmcquade jfmcquade commented Mar 14, 2024

PR Checklist

  • PR title descriptive (can be used in release notes)

Description

Adds the capacitor-managed iOS build of the app. The iOS build has not been extensively tested, so there are bugs and features that should be fixed as follow-ups.

In addition to the files generated by running npx cap add ios, the following changes were required:

Testing

An initial iOS build is available in appetize here.

Dev notes

Steps followed:

  1. install xcode
  2. install xcode cli tools
    • Or otherwise configure xcode CLI tools, this worked for me: sudo xcode-select -s /Applications/Xcode.app/Contents/Developer
  3. install cocoapods
    • For me, I needed to install using homebrew, i.e. brew install cocoapods. Installing by other methods caused errors
  4. Run npx cap add ios
  5. Run yarn build
  6. Run npx cap sync
  7. Run npx cap open ios to open the project with the xcode application.
  8. Start the project in emulation via the xcode UI.
    • This would initially not compile as the Firebase secrets file GoogleService-Info.plist is required
      • This file can't be added from the node project directly and must be added through xcode's UI, see this thread. Although that thread does suggest some workarounds, including using the Trapeze project API and using the xcode npm package (not actively maintained).
      • This file does not get committed to the repo, but some references to the file are added to ios/App/App.xcodeproj/project.pbxproj, which is committed
    • As we are at our limit of Firebase apps for a single project (see this mattermost thread), rather than registering a new app in Firebase I added a dummy version of the file from here, which did enable the app to compile in xcode.

With the initial setup done, the workflow for running the app in emulation would be:

  1. yarn build
  2. npx cap sync ios (or if only web code changed, npx cap copy ios)
  3. npx cap open ios

TODO / Follow-Ups

Testing and bug fixes

Now that the app can be ran in emulation, it would be sensible to perform some general functional testing and create issues for any bugs that are discovered. From some cursory testing, I noticed bugs with the following components and functionality, each of which should be investigated further:

  • Header collapse
  • Footer template
  • Toggle bar

Git Issues

Closes #

Screenshots/Videos

Initial run in emulation. Shows the app installing and opening correctly. Issues are immediately obvious with the header, footer and toggle bar.

Initial.emulation.mov

@jfmcquade jfmcquade changed the title Feat: iOS configuration Feat: iOS setup Mar 15, 2024
@jfmcquade jfmcquade changed the title Feat: iOS setup Feat: iOS build Mar 15, 2024
@jfmcquade jfmcquade requested a review from chrismclarke March 15, 2024 16:37
@chrismclarke
Copy link
Member

chrismclarke commented Mar 15, 2024

Did you manage to test crashlytics update on ios/android as part of the pr or is that still required?

Otherwise not much I can do to test from this side as I don't have a mac to run the code on. Would you be able to share a couple of screenshots or videos to get a feel for how things are looking?

From code side I'm guessing it's mostly just generated from capacitor scripts and firebase install instructions, but is there anything specifically you want me to look at?

Otherwise, I assume this PR should be fine just to get this merged in as a core addition and then iterate (once confirmed crashlyitics migration doesn't break things).

I would probably recommend an early priority should be to add an appetize ios github action to make it easier for non-mac owners to do functional review/testing - which might take a little extra work as I think the old repo-based action has been refactored to reusable so will possibly need to adapt so that we can still test from main repo (likely checking out the debug repo content)

@jfmcquade
Copy link
Collaborator Author

jfmcquade commented Mar 18, 2024

Thanks @chrismclarke. Appreciate there's not much you can do to test without being able to run the code.

I've attached a video to the PR description showing an initial run of the app in the iOS emulator. I've also added a link to a build manually uploaded to appetize (available here).

I've also broken out the various follow-ups into issues. They are grouped under the iOS support milestone.

I have now tested crashlytics using the new package on this feature branch for an Android build. Following your suggestion in this PR comment, I hardcoded some method calls to the crashlytics service on startup. I can confirm that the recordException and crash methods trigger events that can be seen in the crashlytics console: here and here. However I can't find the effects of log method in the crashlytics console – is it supposed to show as a non-fatal issue do you know? The wording in the capacitor plugin docs is ambiguous to me about what the expected outcome is.

The method calls I added were as follows:

this.log({message: "Test PR 2234 - log"})
this.recordException({message: "Test PR 2234 - exception"})
this.crash({message: "Test PR 2234 - crash"})

Pending that crashlytics question, I would say I'd be happy to merge this PR as-is if you are, then we can work on the related issues as follow-ups.

@jfmcquade jfmcquade marked this pull request as ready for review March 18, 2024 16:20
@chrismclarke
Copy link
Member

Thanks @jfmcquade , all sounds good to me. Thanks for adding the follow-up issues, I've added a few comments but happy to discuss more as needed and take on whatever you need me to (can decide on next call).

I don't think we ever call the crashlytics log event (we have glitchtip for logging) so I wouldn't be too worries. See a bunch of test issues in non-fatal so assume that's all fine (annoyingly the links you post never quite work as firebase personalise the /u property depending on how many logged in profiles/accounts you have and so always a bit of a crapshoot to figure out

image

@jfmcquade jfmcquade merged commit 48a54af into master Mar 19, 2024
9 checks passed
@jfmcquade jfmcquade deleted the feat/ios branch March 19, 2024 10:32
@jfmcquade jfmcquade mentioned this pull request Mar 25, 2024
2 tasks
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.

2 participants