fix(fetch): fixup generators and add error handling #8395
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/nodejs-website Please review the changes when you have a chance. Thank you! 🙏 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8395 +/- ##
==========================================
- Coverage 76.40% 76.39% -0.01%
==========================================
Files 118 118
Lines 9928 9924 -4
Branches 334 334
==========================================
- Hits 7585 7581 -4
Misses 2341 2341
Partials 2 2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull request overview
This PR refactors data generator functions to use promise chain syntax (.then().catch()) instead of async/await, addressing fetch failures during deployments. The changes improve error handling by ensuring that failed fetch operations return safe default values instead of breaking the build.
Key Changes:
- Refactored
vulnerabilities.mjsandsupportersData.mjsgenerators to use.then().catch()pattern with fallback values - Added
supporters.tstype definitions file and exported it from the types index - Moved
OPENCOLLECTIVE_MEMBERS_URLconstant from inline definition to centralizednext.constants.mjs
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
apps/site/types/supporters.ts |
Adds type definitions for Supporter and OpenCollectiveSupporter |
apps/site/types/index.ts |
Exports the new supporters type module |
apps/site/next.constants.mjs |
Adds OPENCOLLECTIVE_MEMBERS_URL constant for centralized configuration |
apps/site/next-data/generators/vulnerabilities.mjs |
Refactors async function to arrow function with promise chains and error handling |
apps/site/next-data/generators/supportersData.mjs |
Refactors async function to arrow function with promise chains, uses centralized constant |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📦 Build Size ComparisonSummary
Changes➕ Added Assets (1)
➖ Removed Assets (1)
|
|
@nodejs/nodejs-website PTAL |
| source: string; | ||
| }; | ||
|
|
||
| export type OpenCollectiveSupporter = Supporter & { source: 'opencollective' }; |
There was a problem hiding this comment.
nit
| export type OpenCollectiveSupporter = Supporter & { source: 'opencollective' }; | |
| export type OpenCollectiveSupporter = Supporter<'opencollective'>; |
| source: 'opencollective', | ||
| })) | ||
| ) | ||
| .catch(() => []); |
There was a problem hiding this comment.
I don't know if silently ignore the error is a good idea
There was a problem hiding this comment.
What alternative did you have in mind?
There was a problem hiding this comment.
What was the initial problem? I'm not sure I understood it
There was a problem hiding this comment.
Occassionally, the fetch will randomly fail for whatever reason (i.e. network failure, the website is down). This bubbles up and breaks the entire deployment.
There was a problem hiding this comment.
I guess we should let it crash so we know something went wrong, rather than silently deploying a build with missing data
There was a problem hiding this comment.
Yes, absolutely, it should crash, otherwise if this fails... Well, no data.
We could add retry-logic tho
| // Helper function to process version patterns | ||
| const processVersion = (version, vulnerability) => { | ||
| // Handle 0.X versions (pre-semver) | ||
| if (/^0\.\d+(\.x)?$/.test(version)) { |
There was a problem hiding this comment.
Maybe create a constant for these regex?
| } | ||
|
|
||
| export default fetchOpenCollectiveData; | ||
| export default () => |
There was a problem hiding this comment.
Can we go back to async please :D
This PR:
.then().catch()format