Standardize TeamsDropdown component usage - #3135
Conversation
|
|
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/fleet-dm/fleet/Ekacob5JrWj5KSr9f4MLes6HiMh6 |
lukeheath
changed the title
|
@RachelElysia I need to do one more code review / QA pass in the morning before this is ready for formal review, but I thought I'd tag you in case you want to take a look. #2996 ended up being related to the messiness of multiple teams dropdown across different pages. I tried patching it for a quick bug fix, but it was difficult to get it to behave consistently across three different versions. I ended up combining them into the global |
4456bce to
bc34046
Compare
d50bd71 to
07fd862
Compare
lukeheath
changed the title
|
@RachelElysia Okay, this is finally ready for review. Working on this kept uncovering new related bugs, so I've included fixes for three bugs in this PR. There is a fourth bug that I have not addressed yet, because I figured I should draw the line somewhere. I'll be tackling #3154 next. |
|
TODO: Free:
Premium: ✅ - Meets the intended behavior
Everything looks good to me! Just double check the tables that this all was all the correct intended behavior! |
| >(["teams"], () => teamsAPI.loadAll(), { | ||
| enabled: !!isPremiumTier, | ||
| select: (data: ITeamsResponse) => data.teams, | ||
| select: (data: ITeamsResponse) => |
There was a problem hiding this comment.
@lukeheath react-query reruns select each time the data is accessed by the virtual DOM (each render, I believe). Even though this is probably fine because it is going to be a small set of teams to sort, I think as a pattern we should avoid doing sorts and similar operations inside of react-query select. Not an urgent issue but wanted to put it on the radar so we don't end up duplicating this pattern in cases where the data sets are bigger like we've seen with hosts and queries recently.
There was a problem hiding this comment.
Thanks for your feedback! This touches on my newfound passion for good patterns being consistently implemented. Apologies for the novel below, but I've been thinking a lot about this kind of stuff so it's a good opportunity to tackle a good example.
I was curious about this, so I dug in a bit. select does fire on each render, but no more than a sort or similar operation in the body of the function, which also runs on every render. I checked out the React Query docs for select and it says:
"This option can be used to transform or select a part of the data returned by the query function."
Which sounds like its purpose is to transform data being returned by useQuery. A better place could be onSuccess, because that only runs on initial load of the data and we only need to sort once. I tried that initially, but couldn't get the return from onSuccess to pass into the return from useQuery. However, returning from select does return the sort to useQuery, which is in alignment with its stated function in the docs.
I did some searching in the codebase to look for patterns where we're running sort, and found this example you wrote that I thought was pretty clever:
// Sort list of teams the current user has permission to access and set as userTeams.
useEffect(() => {
if (isPremiumTier) {
let unsortedTeams: ITeam[] | null = null;
if (isOnGlobalTeam && teams) {
unsortedTeams = teams;
} else if (!isOnGlobalTeam && currentUser?.teams) {
unsortedTeams = currentUser.teams;
}
if (unsortedTeams !== null) {
const sortedTeams = unsortedTeams.sort((a, b) =>
sortUtils.caseInsensitiveAsc(a.name, b.name)
);
setUserTeams(sortedTeams);
}
}
}, [currentUser, isOnGlobalTeam, isPremiumTier, teams]);
Here you're using an unsortedTeams variable to determine if you've already done the sort so that it only runs once. I think that's a good pattern for us to use when we want to optimize.
What I wonder about is when does it make sense to have the code overhead? The more lines of code we write, the more we have to maintain and reason about when we're reading it later. So I think it's worth considering if the use case warrants the optimization.
If we were to implement this pattern everywhere that a .sort type method is used on render, that would mean everything in the function body, and any function called as a prop, or anything that hooks into context anywhere in its component hierarchy because they all run on render. That would create a lot of code bloat that I don't think would provide enough improvement in performance to be worth it.
I'd argue that the select method is the appropriate place to transform return data from useQuery. If the transform is CPU intensive, use your approach of guarding from multiple runs. If the transform is not CPU intensive and unlikely to ever be CPU intensive, less code can be worth the re-renders. If performance ever does become an issue, of course add the necessary code.
I wanted to share my thoughts while they were fresh. I'm looking forward to chatting more about this, and patterns in general, when you're back. Have a good break! 🌻
There was a problem hiding this comment.
@lukeheath I really like the way you've summed all this up here! A couple more thoughts for future discussion:
onSuccess can be a useful tool where we might want do want to transform the API response without the need for another useEffect. In the example you mentioned, there are multiple dependencies that needed to be consolidated into one effect. In cases where the only dependency is the API response that is the subject of the useQuery hook, I think onSuccess can serve achieve the same effect. In such a pattern, local state can hold transformedData and then onSuccess would do the expensive transform calculation one time (i.e. on success) and then perform the side effect of setTransformedData with the transformed value. So I think that pattern would be relatively low-overhead code that is optimized for compute time (with some tradeoff for memory space). The pattern above can be adapted to useMemo as well. For example, check out PR 2790.
Like you said, for simple cases select is perfectly fine. I like select for cases like selecting a particular property from an object (e.g., data.foo) or simple transforms like casting a number to a string or things like String.toLowerCase() or Array.join() or sorting a list that is reliably small. As lists get bigger or transforms get more intensive, it makes more sense to look for optimizations.
3 participants
For #2996
and #3136
and #3150
Checklist for submitter
If some of the following don't apply, delete the relevant line.