feat: Enable react-native-nitro-fetch (V2)#94469
Conversation
|
@mkhutornyi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
|
chrispader
changed the title
react-native-nitro-fetch (V2)
chrispader
changed the title
react-native-nitro-fetch (V2)react-native-nitro-fetch (V2)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const CONTENT_TYPE_HEADER = 'Content-Type'; | ||
|
|
||
| function buildAuthenticateBody(credentials: Credentials): string { | ||
| const shouldUseNewPartnerName = checkIfShouldUseNewPartnerName(credentials.autoGeneratedLogin); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
buildAuthenticateBody re-implements the Authenticate request payload (partner name/password selection plus referer, platform, api_setCookie, appversion, isFromDevEnv, email) that already exists in enhanceParameters.ts and Reauthentication.ts. The partner-name/password selection block here is an exact copy of Reauthentication.ts:170-173 / Session/index.ts:252-253, so the same logic now lives in three places and can drift independently.
Consider extracting the shared partner-name/password resolution (and, where feasible, the common Authenticate parameter set) into a single reusable helper that both the JS request pipeline and this native-prefetch body builder consume, e.g.:
// src/libs/SessionUtils.ts
function getPartnerCredentials(autoGeneratedLogin?: string) {
const useNew = checkIfShouldUseNewPartnerName(autoGeneratedLogin);
return {
partnerName: useNew ? CONFIG.EXPENSIFY.PARTNER_NAME : CONFIG.EXPENSIFY.LEGACY_PARTNER_NAME,
partnerPassword: useNew ? CONFIG.EXPENSIFY.PARTNER_PASSWORD : CONFIG.EXPENSIFY.LEGACY_PARTNER_PASSWORD,
};
}If the duplication is intentional because the native prefetch must serialize this body before the JS bundle loads (and thus cannot reuse the JS request pipeline), a short comment documenting that constraint would make the intent clear to future maintainers.
Reviewed at: 2f912c9 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
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.
|
…fetchOnAppStart`
![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: 2f912c9592
ℹ️ 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".
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
chrispader
changed the title
react-native-nitro-fetch (V2)react-native-nitro-fetch (V2)
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@mkhutornyi @chrispader The bug is not repro here #92680 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
|
#92652 also not repro |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eadf875939
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| */ | ||
| import {fetch as nitroFetch, Headers as NitroHeaders, Request as NitroRequest, Response as NitroResponse} from 'react-native-nitro-fetch'; | ||
|
|
||
| globalThis.fetch = nitroFetch; |
There was a problem hiding this comment.
On native, replacing the global fetch here also routes local-file helpers such as readFileAsync() through NitroFetch; that helper reads receipts/avatars/documents with fetch(path).then(res => res.blob()). In react-native-nitro-fetch@1.4.3-alpha.1, Response.blob() rebuilds the blob from the response text rather than the original bytes, so binary local files such as PDF receipts or images are corrupted before upload. Keep the platform fetch for local/blob/file URLs or update those helpers to read bytes before enabling the global replacement.
Useful? React with 👍 / 👎.
|
@MelvinBot review |
@chrispader please link to https://github.com/Expensify/Mobile-Expensify/pull/13988 |
|
@MelvinBot review |
|
🚧 roryabraham 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.25-0 🚀
|
|
Deploy Blocker #94986 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.4.25-2 🚀
Bundle Size Analysis (Sentry): |
This reverts the changes introduced by PR Expensify#94469 (merge commit 3d794ae). Co-authored-by: Cursor <cursoragent@cursor.com>
7 participants
@roryabraham
Explanation of Change
(This PR is a follow-up for #88110 after it has been reverted)
Replace native
fetchwithreact-native-nitro-fetch, then prefetches the startupOpenApp/ReconnectApprequest so it can begin before React Native JS is ready.NitroFetch is a native-backed Fetch API implementation built on Nitro Modules. By globally replacing fetch, Headers, Request, and Response on native, existing fetch(...) callsites and dependencies keep the same API while routing through NitroFetch. Web keeps the browser fetch implementation.
For startup,
prefetchOnAppStart(...)stores theOpenApp/ReconnectApprequest with a stableprefetchKey. On the next cold start, native code replays that request before the React Native runtime is loaded. When JS later sends the same request with the same prefetchKey, NitroFetch can serve the prefetched response from its native cache if it is ready, or fall back to the normal network request if it is not.Fixed and tested the following regressions:
Fixed Issues
$ #90023
PROPOSAL: https://expensify.slack.com/archives/C05LX9D6E07/p1778233186728969
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/13951
Tests
Offline tests
None needed.
QA Steps
Run a full QA suite on all flows, since the changes in this PR affect all network requests across the whole app.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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