[WIP] chore: deprecate timing.ts #80659
Conversation
|
|
sosek108
left a comment
sosek108
left a comment
There was a problem hiding this comment.
LGTM overall 🚀
Once concern is about size of the PR. maybe we could split this?
adhorodyski
force-pushed
the
timing-module-deprecate
branch
from
f2737dc to
fc5eead
Compare
melvin-bot
bot
requested review from
Ollyws and
heyjennahay
and removed request for
a team and
heyjennahay
adhorodyski
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc5eead167
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "@react-native-firebase/analytics": "^22.2.1", | ||
| "@react-native-firebase/app": "^22.2.1", | ||
| "@react-native-firebase/crashlytics": "^22.2.1", | ||
| "@react-native-firebase/perf": "^22.2.1", | ||
| "@react-native-google-signin/google-signin": "^10.0.1", |
There was a problem hiding this comment.
Removing @react-native-firebase/perf means the Firebase Performance pod is no longer installed (Podfile.lock drops Firebase/Performance), but ios/NewExpensify/RCTStartupTimer.h still imports FirebasePerformance/FIRPerformance.h. iOS builds that include this native module will fail to compile with a missing module error. Either keep the perf dependency/pod or remove/guard the startup timer’s FirebasePerformance usage on iOS.
Useful? React with 👍 / 👎.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
rlinoz
left a comment
rlinoz
left a comment
There was a problem hiding this comment.
Looks good, let's just update the doc before merging.
src/libs/Firebase/utils.ts
Outdated
There was a problem hiding this comment.
I wonder if this is just the setup we need to add the tags we want to Sentry, for the big accounts. So we could just move this file to telemetry and set the attributes there.
There was a problem hiding this comment.
I guess we can just write that in another PR anyway.
adhorodyski
changed the title
|
@adhorodyski @rlinoz @sosek108 Updates:
|
|
Confirmed that iOS Standalone is also failing on |
|
🚧 @rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 @rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚧 @rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
Hi @adhorodyski, this closed issue is reproducible on the ad-hoc build. |
|
Synced with main, it was already 2d old. Let's rerun the build. |
|
🚧 @rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚧 @rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
@IuliiaHerets can you please use this one? #80659 (comment) I should have the bugs from other issues fixed. |
|
@rlinoz, just to confirm, you’d like to check this issue on the new build? |
|
@rlinoz issue is not repro using the newest build 25.mp4 |
|
@IuliiaHerets yes thank you! and if we could do the rest of the QA in that build, if it didn't get completed in the previous one. |
5 participants
@rlinoz @sosek108
Explanation of Change
This PR deprecates the
Timing.tsmodule as well as any performance tracking done with Firebase. It includes a patch removal.Let's make sure this PR is well tested on an adhoc.
Needs https://github.com/Expensify/Mobile-Expensify/pull/13828 to work with Hybrid.
Fixed Issues
$ #80357
PROPOSAL: N/A
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/13828
Tests
/SendPerformanceTimingscommand have been made.Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari