Denormalize member counts #3977
Conversation
|
|
||
| return { | ||
| count: await getMemberCount(id), | ||
| count: memberCount || 1, |
There was a problem hiding this comment.
Should fallback to await getMemberCount(id) just to make sure?
There was a problem hiding this comment.
Feels hacky, as though we don't trust the migration or the logic in the code to keep this right. Wdyt? Even having the || 1 feels brittle, but is a worst-case fallback right?
There was a problem hiding this comment.
Yeah but why not make the worst-case correct-but-slow instead of completely-wrong-but-fast? Don't quite understand what the downside is there
There was a problem hiding this comment.
It just feels like this is going to be confusing to us in the future: "why does this one specific resolver fall back from an object field to a full db query?"
Just seems like we're overdoing this resolver with code that feels overly specific and will be confusing to maintain in the future. Technically it's not bad, but we'd also want to know if it ever had to run the fallback code right? That would mean we have bad db data
api/queries/channel/metaData.js
Outdated
| return { | ||
| threads: threads ? threads.reduction : 0, | ||
| members: members ? members.reduction : 0, | ||
| members: memberCount || 1, |
There was a problem hiding this comment.
Should probably fallback to loaders.channelMemberCount.load just to make sure?
|
Kk @mxstbr I copied over your logic for the fallback. This PR is ready for another thorough review :) |
Status
Deploy after merge (delete what needn't be deployed)
Run database migrations (delete if no migration was added)
YES
This is ready for review @mxstbr :)