-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create HOC for get report data or navigate to home #12089
Merged
Beamanator
merged 3 commits into
Expensify:main
from
mollfpr:fix-report-undefined-crash
Oct 28, 2022
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import PropTypes from 'prop-types'; | ||
import React, {Component} from 'react'; | ||
import {withOnyx} from 'react-native-onyx'; | ||
import _ from 'underscore'; | ||
import getComponentDisplayName from '../../../libs/getComponentDisplayName'; | ||
import Navigation from '../../../libs/Navigation/Navigation'; | ||
import ONYXKEYS from '../../../ONYXKEYS'; | ||
import reportPropTypes from '../../reportPropTypes'; | ||
|
||
export default function (WrappedComponent) { | ||
const propTypes = { | ||
/** The HOC takes an optional ref as a prop and passes it as a ref to the wrapped component. | ||
* That way, if a ref is passed to a component wrapped in the HOC, the ref is a reference to the wrapped component, not the HOC. */ | ||
forwardedRef: PropTypes.func, | ||
|
||
/** The report currently being looked at */ | ||
report: reportPropTypes, | ||
}; | ||
|
||
const defaultProps = { | ||
forwardedRef: () => {}, | ||
report: {}, | ||
}; | ||
|
||
class WithReportOrNavigateHome extends Component { | ||
componentDidMount() { | ||
if (!_.isEmpty(this.props.report)) { | ||
return; | ||
} | ||
Navigation.dismissModal(); | ||
} | ||
|
||
render() { | ||
const rest = _.omit(this.props, ['forwardedRef']); | ||
|
||
return ( | ||
<WrappedComponent | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...rest} | ||
ref={this.props.forwardedRef} | ||
/> | ||
); | ||
} | ||
} | ||
|
||
WithReportOrNavigateHome.propTypes = propTypes; | ||
WithReportOrNavigateHome.defaultProps = defaultProps; | ||
WithReportOrNavigateHome.displayName = `withReportOrNavigateHome(${getComponentDisplayName(WrappedComponent)})`; | ||
const withReportOrNavigateHome = React.forwardRef((props, ref) => ( | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
<WithReportOrNavigateHome {...props} forwardedRef={ref} /> | ||
)); | ||
|
||
return withOnyx({ | ||
report: { | ||
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`, | ||
}, | ||
})(withReportOrNavigateHome); | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch, did we really need an HOC for this? Very confused about what purpose this even serves. @Beamanator would you mind summarizing why we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is your question "Why a HOC for this functionality" or "What is the point of the code in this HOC"? Will happily respond to either one tomorrow (end of my day & brain is foggy from timezone shifting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes to both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"What is the point of the code in this HOC" should be pretty straightforward: If the user is on a page that needs
report
data (from Onyx), we check ifthis.props.report
is empty or not. If it's empty, we dismiss the open modal (this is what we were calling "navigate home" though that's not really accurate since we don't have a "home" screen...). If it's not empty, we show that page."Why a HOC for this functionality": We wanted to use this functionality across multiple pages, so thought it was clean to put it in an HOC. This lets us not have to subscribe to
${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}
, on multiple pages, and instead we just wrap the page with this new HOC. We could do the same thing with a newaction
orlib
that we call whenever these pages are mounted, but in my brain an HOC seemed more useful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks and sorry for my initial reaction. I think we got the main solution for this wrong which is the important thing. I tend to only pull out the HOC when we the code reuse is expected to be very high for a solution. But if you thought we might re-use this then the HOC is certainly one way to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're referring to the fact that we aren't actually sure if we want to navigate at this point, right? Like we probably want to see if the user has access to that report or not before navigating away? (this is what our other discussion is about in a different thread, I believe)
Yeah exactly that was my thought process too, BUT maybe you would have waited till we have a few more use cases for this kind of code before making it a HOC - I'll happily defer to you on this, though I guess the "next steps" probably depend on what the "correct solution" is (again referring to our other discussion - showing a "you don't have accesss" page or navigating somewhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I think there are different possible states of report access and different app states depending on if those reports were previously accessible or not.
Using the existence of the report in storage doesn't tell us whether you have "access" or not especially when you are offline...
Offline and never attempted to fetch a report: No way to really tell if the user has access or not but that's fine the same error message works in this case.
Offline and previous attempt returned 404: Tell them they can't access it i.e. "Hmm... it's not here"
Online and attempt returns 404: Tell them they can't access it i.e. "Hmm... it's not here"
I don't think there is any situation where we'd want to redirect without any explanation (but this deserves a wider discussion to get more input than just me 😅).