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

Reduce GitHub API calls in issues reporter #1113

Merged
merged 7 commits into from
Oct 23, 2024
Merged

Reduce GitHub API calls in issues reporter #1113

merged 7 commits into from
Oct 23, 2024

Conversation

Ndpnt
Copy link
Member

@Ndpnt Ndpnt commented Oct 23, 2024

No description provided.

@Ndpnt Ndpnt requested a review from MattiSG October 23, 2024 13:50
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Thanks for this work!
The caching mechanism is a bit too eager from my perspective: I believe we would make it significantly simpler by simply having a getter that populates when it is requested for the first time, and better encapsulating its updates 🙂

src/reporter/github.js Outdated Show resolved Hide resolved
}
}

async ensureCacheLoaded() {
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply await this.cacheLoadedPromise when we call ensureCacheLoaded()? 🙂 Then we don't need to await it in refreshIssuesCache() and we don't need this method.


return issue;
getIssue(title) {
return this.issuesCache.get(title) || null;
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't undefined acceptable as a return value if the issue is not found in the cache? 🙂

Suggested change
return this.issuesCache.get(title) || null;
return this.issuesCache.get(title);

src/reporter/github.js Outdated Show resolved Hide resolved
src/reporter/github.js Show resolved Hide resolved
src/reporter/github.test.js Outdated Show resolved Hide resolved
src/reporter/github.test.js Outdated Show resolved Hide resolved
src/reporter/github.test.js Outdated Show resolved Hide resolved
src/reporter/github.test.js Outdated Show resolved Hide resolved
@@ -49,6 +49,10 @@ export default class Reporter {
return this.github.initialize();
}

onTrackingStarted() {
return this.github.refreshIssuesCache();
Copy link
Member

Choose a reason for hiding this comment

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

Why be eager? Let's just wait until we actually need the cache 🙂

@Ndpnt Ndpnt requested a review from MattiSG October 23, 2024 15:36
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Looks really good, thanks!!

@Ndpnt Ndpnt merged commit b120a3d into main Oct 23, 2024
8 checks passed
@Ndpnt Ndpnt deleted the cache-issues branch October 23, 2024 15:55
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.

2 participants