-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
[winget] Add optional ReleaseDate to WinGet version badge #11622
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
base: master
Are you sure you want to change the base?
[winget] Add optional ReleaseDate to WinGet version badge #11622
Conversation
Fixes badges#11285 Contribution by Gittensor, see my contribution statistics at https://gittensor.io/miners/details?githubId=105917501
|
Contribution by Gittensor, see my contribution statistics at https://gittensor.io/miners/details?githubId=105917501
Contribution by Gittensor, see my contribution statistics at https://gittensor.io/miners/details?githubId=105917501
|
@theflow would you review this PR? |
|
Hello @josephadamsdev 👋🏻 Thanks for looking into this. Our badges generally perform a single upstream API request underneath the hood.
Is there no way to retrieve the version number and its release date in a single API call to GitHub? |
Address performance concerns raised in PR review by reducing API calls from N+1 to 2 requests total. Previous implementation: - Made 1 GraphQL call to get version directories - Made 1 additional GraphQL call PER manifest file to check for ReleaseDate - Could result in 3-5+ API calls for packages with multiple manifest files - Response times approaching 3 seconds Optimized implementation: - Make 1 GraphQL call to get version directories - Make 1 GraphQL call to fetch ALL manifest files at once - Parse files in memory to find ReleaseDate - Total: 2 API calls regardless of manifest count - Expected to reduce response time by 50%+ for release date badges Changes: - Replace fetchManifestText() with fetchAllManifests() that fetches entire directory in single GraphQL query - Update multiManifestSchema to handle array of blob entries with text - Update test to match new response format - Remove unused manifestSchema I addressesed the maintainer's concern about API rate limits and response time approaching GitHub's Camo proxy timeout threshold.
|
@PyvesB Thanks for the feedback I've addressed the performance and rate limiting concerns. What ChangedI've optimized the implementation to reduce API calls from N+1 to exactly 2 total requests: Before:
After:
Implementation DetailsThe new TestingAll 5 unit tests passing, including the release date test that validates the optimization works correctly. This should address your concerns about Camo timeout threshold and rate limit constraints. Let me know if you'd like me to make any additional improvements |
Summary
Add an optional
include_release_datequery param to the WinGet version badge to append the manifestReleaseDate(when available).Related Issue
Fixes #11285
Testing
npx eslint services/winget/winget-version.service.js services/winget/winget-version.tester.jsUnable to run
npm run test:serviceslocally because this environment blocks binding to localhost ports.Contribution by Gittensor, see my contribution statistics at https://gittensor.io/miners/details?githubId=105917501