Repository navigation
[No QA] TSConfig cleanup – Phase 2: fix third-party typing so the web tsconfig has no errors - #105816
Conversation
|
|
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.
|
…e2-99287/third-party-types
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| return Buffer.from(value).toString('base64').replaceAll('+', '-').replaceAll('/', '_').replace(/=+$/, ''); | ||
| encode: (value: string | Uint8Array): Base64URLString => { | ||
| // `Buffer.from` only has separate overloads for strings and byte arrays, so the union has to be narrowed first. | ||
| const bytes = typeof value === 'string' ? Buffer.from(value) : Buffer.from(value); |
There was a problem hiding this comment.
The two Buffer.from(value) branches read like a copy-paste slip, and someone could "simplify" them back later and break the typecheck. Could you double check?
There was a problem hiding this comment.
this code does look pretty bizarre
There was a problem hiding this comment.
I changed to be more visually distinct
encode: (value: string | Uint8Array): Base64URLString => {
// `Buffer.from` has separate overloads for strings and byte arrays, so each type needs its own call.
const bytes = typeof value === 'string' ? Buffer.from(value, 'utf8') : Buffer.from(value);
return bytes.toString('base64').replaceAll('+', '-').replaceAll('/', '_').replace(/=+$/, '');
},About the reason, right now isn't a problem but with the upcoming native tsconfig it won't accept the current string | Uint8Array in Buffer.from anymore in the way it's accepting today, so we need to use a condition to narrow the type.
To make both Buffer.from distinct from each other I'm passing utf8 to the first one, which is right and it's the default parameter for strings anyway.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeScreen.Recording.2026-11-06.at.01.34.05.moviOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-11-06.at.01.26.41.mov |
dylanexpensify
left a comment
dylanexpensify
left a comment
There was a problem hiding this comment.
product input not required
8f966c3 to
e8dd8d0
Compare
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 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.5.34-0 🚀
|
|
No help site update needed — this PR corrects internal TypeScript typings and platform file organization without adding a user-facing workflow, changing UI labels, removing a beta, or making existing article instructions incorrect. |
|
No FullStory changes needed — this PR fixes TypeScript typings and refactors platform-specific code without adding user-facing features, interactions, screens, or sensitive content. |
|
Deploy Blocker #106350 was identified to be related to this PR. |
After investigation I have initial findings that its might not be this pr but #104479 as 104479 one did touch the share feature while this one only related to tscofig also the author already commented about it #106350 (comment) |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.5.34-1 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Part of the tsconfig cleanup, phase 2 (web/native split). The upcoming web tsconfig uses
moduleSuffixes: [".web", ""], which also applies insidenode_modules. So it typechecks against the libraries'.web.d.tsfiles, and some of those have bugs. This PR fixes the third-party typing so the web tsconfig has no errors when the switch lands. Measured with the web/native tsconfigs applied, web goes from 74 errors to 4. The remaining 4 belong to the switch PR: thesrc/typesmove and thereact-nativeexport condition.Patches (each one documented in its
details.md):react-native-svg001: webDefsacceptschildren. Fixed upstream in fix: allow children on web Defs software-mansion/react-native-svg#3056, not released yet.react-native-svg002: webBaseProps.stylewas typedIterable<unknown>, but the web runtime accepts plain style objects too.react-native-gesture-handler002: the web v3FlatList/TextInputget the same type exports as native, andFlatListgets itsrefprop.expo-image-manipulator003: the web module is typed as an instance, not the class (fixed upstream inexpo-modules-coreby [web][modules-core] TyperegisterWebModuleas returning a module instance expo/expo#49197), and webresize()makeswidth/heightoptional like native.expo-video002: the webVideoViewref handle is typed, andplayeraccepts the publicVideoPlayertype.react-native-nitro-fetch002:Response.redirect()acceptsstring | URLlike the standard, converting withString(url). This is the only runtime change in the patches, in a method the App doesn't call.App code:
react-native-svg.d.ts: declares theSvgPropsthe App passes to<Svg>, because react-native-svg's web entry exports no prop types.ComponentRef<typeof LottieView>.SkeletonRecttakes its props fromComponentProps<typeof Rect>, because there is noRectPropson web.registerPrefetchOnAppStart: the native implementation moves toindex.native.ts, and the web no-op becomesindex.ts. Every platform keeps the same implementation (rsbuild picks the no-op; Metro and Jest pick the native one).swipeThresholdinReanimatedModaltypes,Promise<void>inusePressLoading,Base64URL.encodenarrowed tostring | Uint8Array, andmarginBottom: 0on inline images inuseMarkdownStyle(live-markdown's own default, so nothing changes visually).Fixed Issues
$ #99287
Tests
N/A
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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.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