Skip to content
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

counts_by_article_type as a protobuf #921

Merged
merged 10 commits into from
Feb 7, 2025
Merged

counts_by_article_type as a protobuf #921

merged 10 commits into from
Feb 7, 2025

Conversation

kavigupta
Copy link
Owner

@kavigupta kavigupta commented Feb 6, 2025

Resolves #830

@@ -407,6 +409,7 @@ export async function loadPageDescriptor(newDescriptor: PageDescriptor, settings
data: await data,
renderedStatname: newDescriptor.statname,
universe: statUniverse,
counts: await counts(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the await counts() above it might fire off two calls.... either confirm the browser is smart enough to cache & combine them, or prefer one const countsResult = await counts() above. (Naming should probably be const counts = await getCounts()

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait it's cached in the ts though right?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh missed this. In this case prefer making loadArticle async using getCountsByArticleType() within that function. This reduces complexity by moving code that deals with article type counts closer to where we need to care about it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this line you can add a simple await getCountsByArticleType() rather than needing a separate variable.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StatisticPanel needs this to compute the set of universes to display, I've added a comment to this effect

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed the redundancy of counts as a variable

@kavigupta kavigupta merged commit 889b6d1 into master Feb 7, 2025
39 checks passed
@kavigupta kavigupta deleted the counts-proto branch February 7, 2025 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

counts_by_article_type as protobuf
2 participants