-
Notifications
You must be signed in to change notification settings - Fork 3k
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: setup background task from scratch on iOS #53834
feat: setup background task from scratch on iOS #53834
Conversation
4822dc1
to
9364f0d
Compare
ios/NewExpensify/Info.plist
Outdated
</array> | ||
<key>BGTaskSchedulerPermittedIdentifiers</key> | ||
<array> | ||
<string>com.expensify.chat.dev</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: which one should we specify here?
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
🚧 @thienlnam has triggered a test build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
2ee195e
to
240573a
Compare
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@mananjadhav any updates? |
Will be checking in next 10 mins. I did check this yesterday and the messages didn't arrive in like 5-8 mins. I couldn't check after that. But I'll be doing another test today. |
@szymonrybczak I tried to test this and it didn't work for me. I sent a message at 21:55 my time and it's 22:13 my time right now. No messages. I sent 10-13. I tried different variations and I checked on two iPhones by the way - iPhone X and iPhone 16 plus. ios-background-to-foreground.MP4 |
@szymonrybczak any updates on this one? |
hey @mananjadhav, to answer your question and how it should work, so iOS is very strict regarding executing any kind of background tasks and 15 minutes which are used as earliest begin date are indicating first possible execution of the task. As mentioned in the docs:
So actually we don't have any guarantee on when it will be executed, as other doc says:
To validate the implementations locally there's this guide. I followed steps from it and aded a video inside this PR description which presents that for specific identifier correct callback is being executed on JS side. I spent a lot of time of trying to find a way to test it inside a Release build, searched a lot of native-only examples but there's no other way for To test how big impact does it have on app experience I'd propose sending analytics when the background task is executed, so we'll have a clear picture on the reliability of this solution and how often iOS decides to execute it. |
4fe46ab
to
6276845
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patience here @szymonrybczak. Checklist completed here. I am not sure what the Comment on native files changed
means. @mountiny Can you help?
We did not find an internal engineer to review this PR, trying to assign a random engineer to #50140 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB maybe we should leave a comment on why src/setup/backgroundTask/index.ts is empty so people don't delete it
One of the checks is failing:
That's not a blocker to merge this PR. |
Congrats, that's your 5th PR merged! 🎉 Do you know about the ContributorPlus role? It's an opportunity to earn more in the Expensify Open Source community. Keep up the great work - thanks! |
@luacmartins looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
See above |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.86-0 🚀
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.86-0 🚀
|
I've added it in Android PR: 7472c60 |
Thanks! |
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.86-3 🚀
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szymonrybczak any reason we committed this file to the repo? Normally it's just a temporary file used in production builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take a look at this, probably remove it and add it into the .gitignore, thanks for caching this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on this in this PR: #56169
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks but I already got to it in this PR: #57058
Explanation of Change
Warning
This Pull Request is rebased on top of the Upgrade to React Native 0.76 Pull Request because it's using a new type-safe way of creating EventEmitters available only in 0.76.
Adds an implementation of background task on iOS. This implemenation leverages BGTaskScheduler API, which is iOS specific API available from iOS 13 responsible for scheduling and executing background tasks. For our specific use case we're using BGProcessingTask which is designed for executing longer tasks e.g. syncing requests as in our case that can take longer.
From the more technical perspective it's a C++ TurboModule that was bootstrapped as a Local Library so it's linked inside
package.json
.For now, we're scheduling new tasks in 15-minute intervals after the execution of the previous one.
How to test it?
To force execution of the background task can use Xcode debugger attached to physical iPhone device:
Here's a video presenting this behaviour:
CleanShot.2024-12-10.at.11.59.35.mp4
Fixed Issues
$ #50140
PROPOSAL: #50140
Tests
Offline tests
QA Steps
Note: take into account that could some time for the first time, this times will be improved and vary depending on the app usage on the device. We set the interval between tasks to 15 minutes but at the end of the day iOS decides when to execute it.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop