[NO QA] chore: upgrade TypeScript to version ^6.0.2#87771
[NO QA] chore: upgrade TypeScript to version ^6.0.2#87771roryabraham merged 20 commits intoExpensify:mainfrom
Conversation
|
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
dariusz-biela
changed the title
staszekscp
left a comment
staszekscp
left a comment
There was a problem hiding this comment.
Looking good! Please, only have a look at the node compatibility that Rory has pointed out
melvin-bot
Bot
requested review from
a team,
DylanDylann and
joekaufmanexpensify
and removed request for
a team
|
@DylanDylann 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] |
|
@youssef-lr 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] |
![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: 9e8e99debf
ℹ️ 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".
|
|
||
| useEffect(() => { | ||
| if (!email || !primaryContactMethod || primaryContactMethod.toLowerCase() !== email.toLowerCase() || isWorkEmailValidated) { | ||
| if (primaryContactMethod?.toLowerCase() !== email?.toLowerCase() || isWorkEmailValidated) { |
There was a problem hiding this comment.
This condition is no longer equivalent to the previous one: when both email and primaryContactMethod are empty strings, primaryContactMethod?.toLowerCase() !== email?.toLowerCase() evaluates to false, so the effect now falls through and navigates to the verify-work-email route even though no email was entered. Because usePrimaryContactMethod() defaults to '', this can happen on initial render/loading and incorrectly redirect users from the add-work-email page.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
we have to be careful with eslint auto fix in this case, because sometimes it can make this stupid logical error
|
@dariusz-biela ESLint is failing |
dariusz-biela
force-pushed
the
dariusz-biela/chore/upgrade-ts-6
branch
from
688761b to
c38c04e
Compare
|
The history of this branch has diverged from main (which is why there are so many change lines in it), I will fix it tomorrow. |
|
No product review needed |
Remove baseUrl, downlevelIteration, allowSyntheticDefaultImports, esModuleInterop (all deprecated/redundant in TS6). Migrate module and moduleResolution from commonjs/node to node16/node16.
Fix prefer-optional-chain violations in ReportActionsUtils, Policy.ts, and Report/index.ts detected by typescript-eslint 8.58.1.
Fix prefer-optional-chain in ReportUtils, IOU/index, SplitExpensePage, and WorkspacesListPage.
…-effect Derive selectedOption from state fallback chain instead of syncing via useEffect, fixing new eslint rule violation.
- useSearchBulkActions: simplify !policy || !policy.achAccount to !policy?.achAccount - WorkspaceExpensifyCardAddWorkEmailPage: suppress with eslint-disable, optional chain changes logic when both values are falsy
Same prefer-optional-chain autofix bug: optional chain drops early return when both email and primaryContactMethod are falsy, causing incorrect navigation.
dariusz-biela
force-pushed
the
dariusz-biela/chore/upgrade-ts-6
branch
from
c38c04e to
7583328
Compare
Optional chain removes the empty-string guard for session.email, causing a false positive "same as signup email" error when both emails are empty.
|
I performed a rebase and fixed the Git history. Then I manually checked all instances of Then I ran Claude Code to check all the changes for maintaining the previous logic, and it didn’t find any additional errors. I believe this PR is fully ready for final review. |
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
same for the other sites where lint is disabled - I'm confident there are solutions that maintain existing logic without requiring us to suppress lint.
| const domain = emailParts.at(1) ?? ''; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/prefer-optional-chain -- optional chain removes the falsy guard for empty strings, causing a false positive when both emails are empty | ||
| if (session?.email && userEmail.toLowerCase() === session.email.toLowerCase() && !isOffline) { |
There was a problem hiding this comment.
I'm on a mission to denormalize suppressing lint - let's avoid it in this case because there's another way:
| if (session?.email && userEmail.toLowerCase() === session.email.toLowerCase() && !isOffline) { | |
| if (session?.email !== "" && userEmail.toLowerCase() === session?.email.toLowerCase() && !isOffline) { |
The rule's --fix introduces logic bugs by converting falsy guards to optional chains, removing protection against empty strings and nullish values. Workarounds (eslint-disable comments, magic-string hacks) are worse than the original code. Disabling the rule and removing all related eslint-disable comments.
|
Awaiting C+ checklist from @DylanDylann |
| function getXeroTenants(policy: Policy | undefined): Tenant[] { | ||
| // Due to the way optional chain is being handled in this useMemo we are forced to use this approach to properly handle undefined values | ||
| // eslint-disable-next-line @typescript-eslint/prefer-optional-chain | ||
| if (!policy || !policy.connections || !policy.connections.xero || !policy.connections.xero.data) { | ||
| return []; | ||
| } | ||
| return policy.connections.xero.data.tenants ?? []; | ||
| } |
There was a problem hiding this comment.
function getXeroTenants(policy: Policy | undefined): Tenant[] {
return policy?.connections?.xero?.data?.tenants ?? [];
}
There was a problem hiding this comment.
@dariusz-biela Is there any reason why we don't update this place?
There was a problem hiding this comment.
That was an implementation from before this PR, which is why I missed the opportunity to optimize it.
We can actually simplify this.
I've already added a commit for this.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
One minor comment: #87771 (comment) The rest looks fine to me |
Replace verbose nullish guards with optional chaining as the behavior is identical — all intermediate properties are objects, so falsy checks and optional chaining handle the same undefined/null cases.
|
🚧 @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. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.61-0 🚀
Bundle Size Analysis (Sentry): |
|
This PR upgrades TypeScript from 5.9.x to 6.0.2 and updates related build tooling ( No help site documentation changes are required. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.3.61-4 🚀
|
7 participants
Explanation of Change
Upgrade TypeScript from 5.9.x to 6.0.2 and update related tooling to ensure compatibility.
Core changes:
typescriptto^6.0.2typescript-eslintto^8.58.1(TS6 support since 8.58.0)ts-jestto^29.4.9(TS6 support since 29.4.7)TS6 breaking changes addressed:
noUncheckedSideEffectImportsnow defaults totrue: Addedsrc/types/modules/css.d.tsto declare*.cssmodule types for all CSS side-effect imports (mapbox, onfido, storybook)baseUrldeprecated: RemovedbaseUrlfromtsconfig.jsonand subsidiary tsconfigs. Moved thetests/*path alias (previously relying onbaseUrl) into explicitpathsmappingrootDirdefault changed from inferred to.: Added explicitrootDirtoscripts/tsconfig.jsonand roottsconfig.json'sts-nodesection, sincets-nodeinternally disablesnoEmitwhich triggers TS6's rootDir enforcementallowSyntheticDefaultImports(always true in TS6),DOM.Iterablefrom lib (included in DOM in TS6),downlevelIteration, and migratedmoduleResolution: "node"to"node16"in subsidiary tsconfigsprefer-optional-chain:typescript-eslint8.58.1 with TS6's deeper type analysis detects more cases — fixed all violations across ~70 filesConfig consolidation:
tsconfig.tsgo.json—tsgonow uses the maintsconfig.jsondirectly with CLI flags (--noEmit --incremental --tsBuildInfoFile), eliminating config driftFixed Issues
$ #83349
PROPOSAL:
Tests
Offline tests
N/A - This is a build tooling upgrade with no runtime behavior changes.
QA Steps
N/A - This is a build tooling upgrade with no runtime behavior changes.
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
iOS: Native
MacOS: Chrome / Safari