fix/28334: Missing displayName prop leads to console error (testID undefined)#28813
fix/28334: Missing displayName prop leads to console error (testID undefined)#28813roryabraham merged 3 commits intoExpensify:mainfrom
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@sobitneupane 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] |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
Is it possible to set
|
There was a problem hiding this comment.
Oh, I'm a bit confused here. We've already set the display name for the component here. Why do we need to set displayName for the ref as well?
@roryabraham Is it expected change?
There was a problem hiding this comment.
The functions returned by React.forwardRef don't have an inherent name, therefore a displayName is required considering the new eslint rule added: "react/display-name": 'error', otherwise it would throw lint error.
As soon as this is sorted out, I have @Pujan92's requested modifications in a stash, ready to be pushed, and will also sort the existing conflict.
I didn't want to mess anything up by pushing to the PR before the initial review.
There was a problem hiding this comment.
@ikevin127 I would like to know @roryabraham's thought before proceeding.
There was a problem hiding this comment.
Sorry for the long delay in replies here, I think it's fine that we set a displayName on the wrapper. This looks a bit weird but makes sense when you think about it. It will just show as AddressSearchWithRef in the React Dev Tools inspector instead of Anonymous.
There was a problem hiding this comment.
An alternative here might be to move the React.forwardRef to the initial function declaration instead of having a separate constant for it. Like this:
diff --git a/src/components/AddressSearch/index.js b/src/components/AddressSearch/index.js
index baa03ecc3a..d7d8cd5462 100644
--- a/src/components/AddressSearch/index.js
+++ b/src/components/AddressSearch/index.js
@@ -140,7 +140,7 @@ const defaultProps = {
// Do not convert to class component! It's been tried before and presents more challenges than it's worth.
// Relevant thread: https://expensify.slack.com/archives/C03TQ48KC/p1634088400387400
// Reference: https://github.com/FaridSafi/react-native-google-places-autocomplete/issues/609#issuecomment-886133839
-function AddressSearch(props) {
+const AddressSearch = React.forwardRef((props, ref) => {
const [displayListViewBorder, setDisplayListViewBorder] = useState(false);
const [isTyping, setIsTyping] = useState(false);
const [isFocused, setIsFocused] = useState(false);
@@ -401,19 +401,8 @@ function AddressSearch(props) {
}}
textInputProps={{
InputComp: TextInput,
- ref: (node) => {
- if (!props.innerRef) {
- return;
- }
-
- if (_.isFunction(props.innerRef)) {
- props.innerRef(node);
- return;
- }
-
- // eslint-disable-next-line no-param-reassign
- props.innerRef.current = node;
- },
+ // eslint-disable-next-line no-param-reassign
+ ref: (node) => (ref.current = node),
label: props.label,
containerStyles: props.containerStyles,
errorText: props.errorText,
@@ -493,23 +482,10 @@ function AddressSearch(props) {
{isFetchingCurrentLocation && <FullScreenLoadingIndicator />}
</>
);
-}
+});
AddressSearch.propTypes = propTypes;
AddressSearch.defaultProps = defaultProps;
AddressSearch.displayName = 'AddressSearch';
-const AddressSearchWithRef = React.forwardRef((props, ref) => (
- <AddressSearch
- // eslint-disable-next-line react/jsx-props-no-spreading
- {...props}
- innerRef={ref}
- />
-));
-
-AddressSearchWithRef.displayName = 'AddressSearchWithRef';
-
-export default compose(
- withNetwork(),
- withLocalize,
-)(AddressSearchWithRef);
+export default compose(withNetwork(), withLocalize)(AddressSearch);
Maybe we should discuss in slack
|
Pushed @Pujan92's requested changes and resolved conflict. |
|
@ikevin127 Looks like there was some issue while resolving the conflict. I cannot run the app with error "/Users/sobitneupane/Documents/Expensify/App/src/components/ValuePicker doesn't exist" |
|
@sobitneupane I added back the src/components/ValuePicker/ and its files. It should work now. |
|
I will be OOO next week. @mananjadhav will be taking over the issue.
I believe we have removed |
|
@sobitneupane @mananjadhav Please let me know if everything is all right for merge. |
|
@ikevin127 I am trying to review this PR and it has 621 files. I can see the updates such as |
|
@mananjadhav Sure, I just reverted the rebase I did so that you can review only the changes I made. Again, all tests and lint pass except lint for rulesdir/display-name-property for which I have a PR opened Expensify/eslint-config-expensify#76 to remove the rule as part of this issue. |
|
Thanks @ikevin127 I am half way through the PR, but we've got some merge conflicts to resolve. |
|
@mananjadhav Cool, please let me know how you want me to approach the conflicts. Deal with them now or after you're done with the review ? If now, now should I fix the conflicts such that there won't be 600+ changed files again ? Which would make the PR difficult to review. |
|
@ikevin127 Fixing the conflicts before the review would be helpful. As I can see the changes are repetitive so once I am done with the review, I'll head for the testing.
Yes that is always what we should aim for. |
|
Resolved the merge conflicts. cc @mananjadhav |
|
@mananjadhav Any updates with regards to the review progress ? |
There was a problem hiding this comment.
I don't see why we need this change?
src/components/ValuePicker/index.js
Outdated
There was a problem hiding this comment.
here too new line removed?
There was a problem hiding this comment.
pardon my ignorance, but did we need to update the anon methods too?
There was a problem hiding this comment.
Indeed, that would be required because otherwise the new eslint rule added would throw error:
mananjadhav
left a comment
mananjadhav
left a comment
There was a problem hiding this comment.
Small minor non-blocking comments. Except for one clarification related to the anon methods.
|
I just reviewed and we've got some conflicts again 🤦 Can you please help resolve them? I think once we resolve the conflicts and comments, we'll be good to finish the checklist by tomorrow. |
|
@mananjadhav Fixed instances mentioned in your comments, except for the conflict of file This file seems to have been removed from main, but if I remove it in this PR the app would crash because the hook is used within other files. How do contributors usually approach this case in their PR's ? |
|
@ikevin127 I am not sure I understand. If
But now when you pull, aren't you seeing that the hook usage is removed from other files? Can you share an example of the file? |
ikevin127
force-pushed
the
fix/28334
branch
2 times, most recently
from
be7b282 to
da74a43
Compare
|
@mananjadhav Ready for review / checklist. Hopefully no more conflicts 🤞 |
Reviewer Checklist
Screenshots/VideosWebweb-display-name.movMobile Web - Chromemweb-chrome-display-name.movMobile Web - Safarimweb-safari-display-name.movDesktopdesktop-display-name.moviOSios-display-name_aDvxTKS6.mp4Androidandroid-display-name_YrLbS4fH.mp4 |
|
@roryabraham Whenever you get to this one, ideally we should merge this PR Expensify/eslint-config-expensify#76 first as per your request here #28334 (comment), then I can update the package-lock on this PR as well because currently we get 6 lint errors because of the |
|
✋ 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: 1.3.91-0 🚀
|
| const NavigationAwareCameraWithRef = React.forwardRef((props, ref) => ( | ||
| <NavigationAwareCamera | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...props} | ||
| forwardedRef={ref} | ||
| /> | ||
| )); |
There was a problem hiding this comment.
This has caused a regression #30373, I dont have full context for PR but i dont see a need for double forwarding ref.
There was a problem hiding this comment.
No excuse, I messed up.
Lots of files changed in this PR and I also changed these 2 NavigationAwareCamera files by mistake when they already had the ref forwarded and display name.
Will create a PR to fix the regression asap.
|
🚀 Deployed to production by https://github.com/marcaaron in version: 1.3.91-8 🚀
|
7 participants
Details
Dev - Home address - AddressPage (and many more) – Missing displayName prop leads to console error (testID undefined).
Fixed Issues
$ #28334
PROPOSAL: #28334 (comment)
Tests
Error: Warning: Failed prop type: The prop
testIDis marked as required inScreenWrapper, but its value isundefined.Offline tests
Error: Warning: Failed prop type: The prop
testIDis marked as required inScreenWrapper, but its value isundefined.QA Steps
Error: Warning: Failed prop type: The prop
testIDis marked as required inScreenWrapper, but its value isundefined.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
web.mp4
Mobile Web - Chrome
mobile-web-chrome.mp4
Mobile Web - Safari
mobile-web-safari.mp4
Desktop
desktop.mp4
iOS
ios.mp4
Android
android.mp4