Jakubkalinski0/fix console errors related to forward ref batch4#68843
Conversation
…y NumberWithSymbolForm
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
cb3d4cf to
43a7411
Compare
SzymczakJ
left a comment
There was a problem hiding this comment.
LGTM, left one small comment
|
@allroundexperts 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppUnable to build android Android: mWeb ChromeScreen.Recording.2025-08-25.at.8.51.40.AM.moviOS: HybridAppScreen.Recording.2025-08-25.at.8.50.27.AM.moviOS: mWeb SafariScreen.Recording.2025-08-25.at.8.49.18.AM.movMacOS: Chrome / SafariScreen.Recording.2025-08-25.at.8.40.05.AM.movMacOS: DesktopScreen.Recording.2025-08-25.at.8.44.54.AM.mov |
mountiny
left a comment
There was a problem hiding this comment.
Thanks! One NAB comment, moving this ahead
| forwardedRef(ref); | ||
| } else if (forwardedRef && 'current' in forwardedRef) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| // eslint-disable-next-line no-param-reassign, react-compiler/react-compiler |
There was a problem hiding this comment.
How to do this to follow the rules of React so we do not have to skip the compiler? We will start enforcing it soon so I think ideally this is fixed as well so we do not skip react compiler
There was a problem hiding this comment.
I will look if it can be done
There was a problem hiding this comment.
@mountiny FYI it appears those actually had to be introduced because of the improper handling of refs that caused issues with the autoFocus (which should have been the first sign of something not being right here 😅). So after the autoFocus fix is merged we will no longer be skipping the compiler 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/mountiny in version: 9.1.99-0 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.1.99-11 🚀
|
Explanation of Change
The whole issue comes from
forwardRefbeing deprecated in the near future of react thus we will no longer be able to access the passedrefbyelement.refasforwardRefis needed for passingrefas a separate attribute. Now in order to pass there reference it has to be passed as one of thepropsso we can access it directly or like that ->element.props.ref.In order to adjust the codebase to new react recommendations in every file that was using
forwardRefthe ref had to be moved to thepropsinstead of being a separate attribute andforwardRefhad to be completely removed.Some files had to be changed due to the changes made in other components. In response to the number of files that needed adjusting the changes are made in batches with this one being
the fourth one.In this batch I mostly took care of
Create Manual Expenseflow. At first it was part of one PR which was supposed to cover wholecreate expenseflow but it grew too big. This batch touches plenty of components responsible for handling input from user.Manual money request->Those required change in order for certain inputs using them to work without
forwardRef->These here had to be changed in response to the changes made in other files. They required only
cosmetic changeswhich cannot produce any issues and thus they require no testing ->Fixed Issues
$ #67033
PROPOSAL:
Tests
Offline tests
SAME AS 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))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
2.mov
MacOS: Desktop