[RW-7167][risk=no] Convert DatasetPage component to React hooks #6648
Conversation
There was a problem hiding this comment.
Sorry for the delay. There is a lot going on here and it's difficult for me to tease apart what is changing with this PR vs just moving around. I will probably have to do another pass over the whole thing only looking at the new code, which might generate a bunch more comments on existing issues. But for now I think I have a few high level ones for your consideration.
Thanks for taking this on.
| showErrorModal, | ||
| workspace, | ||
| }: Props) => { | ||
| let dt: any; |
There was a problem hiding this comment.
This seems suspect, seems like React.Component or JSX.ELement or a similar type should work here (maybe check what type is provided in the ref callback below). A more specific name would definitely be good as well, given how far this definition is from where it's used e.g. dataTableComponent
There was a problem hiding this comment.
Ended up removing this. The only purpose for it was to check if text for column headers overlapped and needed a tooltip, which was complicated and wasn't actually working anyway.
Added a check for overlapping text when hovering over the column header instead.
| const [cohortList, setCohortList] = useState([]); | ||
| const [conceptSetList, setConceptSetList] = useState([]); | ||
| const [crossDomainConceptSetList, setCrossDomainConceptSetList] = useState( | ||
| new Set() | ||
| ); | ||
| const [dataSet, setDataSet] = useState(undefined); | ||
| const [dataSetTouched, setDataSetTouched] = useState(false); | ||
| const [domainValueSetIsLoading, setDomainValueSetIsLoading] = useState( | ||
| new Set() | ||
| ); | ||
| const [domainValueSetLookup, setDomainValueSetLookup] = useState(new Map()); | ||
| const [includesAllParticipants, setIncludesAllParticipants] = | ||
| useState(false); | ||
| const [loadingResources, setLoadingResources] = useState(true); | ||
| const [modalState, setModalState] = useState(ModalState.None); | ||
| const [previewList, setPreviewList] = useState(new Map()); | ||
| const [selectedCohortIds, setSelectedCohortIds] = useState([]); | ||
| const [selectedConceptSetIds, setSelectedConceptSetIds] = useState([]); | ||
| const [selectedDomains, setSelectedDomains] = useState(new Set()); | ||
| const [ | ||
| selectedDomainsWithConceptSetIds, | ||
| setSelectedDomainsWithConceptSetIds, | ||
| ] = useState(new Set()); | ||
| const [selectedPreviewDomain, setSelectedPreviewDomain] = useState( | ||
| Domain.CONDITION | ||
| ); | ||
| const [selectedPrepackagedConceptSets, setSelectedPrepackagedConceptSets] = | ||
| useState(new Set()); | ||
| const [selectedDomainValuePairs, setSelectedDomainValuePairs] = useState( | ||
| [] | ||
| ); | ||
| const [savingDataset, setSavingDataset] = useState(false); |
There was a problem hiding this comment.
There are a lot of state hooks here. As one quick win: there's a category of state hooks here which are basically derived state / lookup optimizations. These are good to have, but I think we can make this dependency more direct:
const selectedDomains = userMemo(() => ...initialization logic..., [selectedDomainValuePairs]);
const domainValueSetLookup = useMemo(() => ...initialization logic..., [selectedDomainValuePairs]);
I haven't used useMemo extensively, so there might be a cleaner way of doing what I'm suggesting too. Thoughts?
As for the rest of this state, I think as a follow-up we should consider looking for opportunities to merge some of these state variables together into larger chunks, e.g. have a single state hook for the transient dataset object. If you see clear opportunities here, go for it - but I'm not going to make the request here since this is already large.
There was a problem hiding this comment.
Agreed, there are a lot of hooks here. For the initial conversion, I basically added a state variable for each property in state from the old class component but I'll look at grouping related variables together.
I'll also look at the additional hooks like useMemo as I haven't used them yet, just useState and useEffect so far. Not sure if it will make it in this pr or a follow up.
| return new Set(domains); | ||
| }; | ||
|
|
||
| const apiEnumToPrePackageConceptSets = ( |
There was a problem hiding this comment.
nit: I'd probably pull this out of the render function at least, maybe out to a utils file or something.
There was a problem hiding this comment.
Moved further up in the file for now to get it out of the component. Filed RW-8358 since there are other components/functions that can be move from this file also.
| v.map((pre) => { | ||
| switch (pre) { | ||
| case PrePackagedConceptSetEnum.BOTH: { | ||
| re.add(PrepackagedConceptSet.PERSON), |
There was a problem hiding this comment.
existing issue, but this seems like a very odd usage of , - I suspect it may have even been a typo for ; originally.
There was a problem hiding this comment.
Yeah, looks like a typo. Kind of surprised this didn't cause any errors/warnings.
| v: Array<PrePackagedConceptSetEnum> | ||
| ): Set<PrepackagedConceptSet> => { | ||
| const re: Set<PrepackagedConceptSet> = new Set<PrepackagedConceptSet>(); | ||
| v.map((pre) => { |
There was a problem hiding this comment.
nit: existing issue, but forEach would be clearer, since the return value is disregarded here
| }; | ||
|
|
||
| const loadDataset = (dataset: DataSet, initialLoad?: boolean) => { | ||
| // This is to set the URL to reference the newly created dataset and the user is staying on the dataset page |
There was a problem hiding this comment.
Existing issue, but this should do it: https://cs.github.com/all-of-us/workbench/blob/f7e0affc8ef836fda3d52a4973a9c411ffb988e4/ui/src/app/routing/workspace-app-routing.tsx#L77-L78
Doesn't need to be in this PR.. given scope.
There was a problem hiding this comment.
The comment in the code here is kind of confusing, but we want to update the url without refreshing the page or remounting the component.
IIUC the purpose of withParamsKey is to force a remount on change of a url param.
There was a problem hiding this comment.
You're right - I misunderstood the intent here. This seems OK then - there might be a cleaner way of doing it but I'd need to see why remounting is a problem first..
| hideSpinner(); | ||
| updatePrepackagedDomains(); | ||
|
|
||
| (async () => await loadResources())(); |
There was a problem hiding this comment.
This should be equivalent to just calling loadResources(), and not waiting on the return promise. Maybe with a comment explaining that you're letting it run in the background
There was a problem hiding this comment.
Yeah, didn't realize on first pass that the async/await here wasn't doing anything, which is an issue when loading a saved dataset. I moved the if statement below into loadResources to make sure all the resources have returned before loading the dataset.
| this.loadValueSetForDomain(nd); | ||
| }); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
nit: for completeness, this should probably list datasetId
|
|
||
| useEffect(() => { | ||
| updatePrepackagedDomains(); | ||
| }, [workspace.namespace, workspace.id, cdrVersionTiersResponse]); |
There was a problem hiding this comment.
This is a bit tricky to follow, presumably the callback uses these variables.
A possible alternative you could try:
const updatePrepackagedDomains = useCallback(() => ..., [[workspace.namespace, workspace.id, cdrVersionTiersResponse]);
useEffect(() => updatePrepackagedDomains(), [updatePrepackagedDomains]);
i.e. updatePrepackagedDomains is only recomputed when its dependencies change. The useEffect is only run when updatePrepackagedDomains is recomputed.
There was a problem hiding this comment.
So I ended up removing this as it doesn't seem necessary. I'm finding a lot of the updates that were in componentDidUpdate were either redundant or never used.
Converts the
DatasetPagecomponent from a class to a function component that uses React hooksPR checklist