feat: Add Cloudflare Workers support + follow-up fixes#4823
feat: Add Cloudflare Workers support + follow-up fixes#4823BBleae wants to merge 2 commits intoanuraghazra:masterfrom
Conversation
…oad worker routes - http.js: use native fetch for non-test envs to avoid axios adapter issues; add normalizeJsonPayload helper for consistent response parsing - retryer.js: normalize non-GraphQL message payloads (e.g. Bad credentials) into GraphQL-like error shape in both success and catch paths - repo.js: add missing imports and handle GraphQL errors from retryer response - worker.js: lazy-load route handlers so process.env is populated before module evaluation; replace switch with route-map lookup; guard process.env from being undefined - tests/pat-info.test.js: preserve NODE_ENV when resetting process.env in beforeAll so http.js continues to use axios mock adapter in tests
|
@BBleae is attempting to deploy a commit to the github readme stats Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
Adds a Cloudflare Workers deployment path for github-readme-stats while adjusting runtime HTTP behavior and error normalization so the existing API handlers can run across environments more consistently.
Changes:
- Introduces a Workers entrypoint (
src/worker.js) plus an Express/Vercel-style adapter (src/common/adapter.js) and Wrangler config (wrangler.toml). - Switches GraphQL HTTP requests to native
fetchat runtime (keeping axios for tests) and normalizes some non-GraphQL HTTP error payloads into a GraphQL-like{ errors: [...] }shape. - Updates repo fetcher error handling and adjusts tests to accommodate the new runtime behavior.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| wrangler.toml | Adds Wrangler configuration for deploying the Worker. |
| package.json / package-lock.json | Adds wrangler dev dependency and lockfile updates. |
| src/worker.js | Worker fetch handler with lazy route loading after env injection. |
| src/common/adapter.js | Adapter that bridges Request/Response to existing (req, res) handlers. |
| src/common/http.js | Uses native fetch outside tests; preserves axios semantics for tests. |
| src/common/retryer.js | Normalizes certain HTTP error payloads into GraphQL-like errors. |
| src/fetchers/repo.js | Adds explicit errors handling with logging + CustomError throwing. |
| src/cards/gist.js / src/cards/wakatime.js | Switches language colors loading approach (JSON import). |
| tests/pat-info.test.js | Preserves NODE_ENV=test so tests keep using axios path. |
| tests/calculateRank.test.js | Makes rank percentile assertion less brittle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Import language colors. | ||
| * | ||
| * @description Here we use the workaround found in | ||
| * https://stackoverflow.com/questions/66726365/how-should-i-import-json-in-node | ||
| * since vercel is using v16.14.0 which does not yet support json imports without the | ||
| * --experimental-json-modules flag. | ||
| */ | ||
| import { createRequire } from "module"; | ||
| const require = createRequire(import.meta.url); | ||
| const languageColors = require("../common/languageColors.json"); // now works | ||
| // import { createRequire } from "module"; | ||
| // const require = createRequire(import.meta.url); | ||
| // const languageColors = require("../common/languageColors.json"); // now works | ||
| import languageColors from "../common/languageColors.json"; |
There was a problem hiding this comment.
import languageColors from "../common/languageColors.json"; will fail under Node ESM unless JSON import attributes/assertions are provided (or the file is converted to a JS module). This also makes the surrounding comment misleading (it describes a createRequire workaround that is now commented out). Please switch to a Node-compatible approach (e.g., JS module export or JSON import with attributes) and update/remove the outdated comment block.
| /** Import language colors. | ||
| * | ||
| * @description Here we use the workaround found in | ||
| * https://stackoverflow.com/questions/66726365/how-should-i-import-json-in-node | ||
| * since vercel is using v16.14.0 which does not yet support json imports without the | ||
| * --experimental-json-modules flag. | ||
| */ | ||
| import { createRequire } from "module"; | ||
| const require = createRequire(import.meta.url); | ||
| const languageColors = require("../common/languageColors.json"); // now works | ||
| // import { createRequire } from "module"; | ||
| // const require = createRequire(import.meta.url); | ||
| // const languageColors = require("../common/languageColors.json"); // now works | ||
| import languageColors from "../common/languageColors.json"; |
There was a problem hiding this comment.
import languageColors from "../common/languageColors.json"; is not valid in Node ESM without JSON import attributes/assertions, so this can throw at runtime/tests. Also the comment block still refers to a createRequire workaround that is now commented out. Please use a Node-compatible way to load this data (e.g., convert the JSON to a JS module export, or use JSON import attributes) and align the comment with the implementation.
| if (res.data.errors) { | ||
| logger.error(res.data.errors); | ||
| if (res.data.errors[0].message) { | ||
| throw new CustomError( | ||
| wrapTextMultiline(res.data.errors[0].message, 90, 1)[0], | ||
| res.statusText, | ||
| ); | ||
| } | ||
| throw new CustomError( | ||
| "Something went wrong while trying to retrieve the repo data using the GraphQL API.", | ||
| CustomError.GRAPHQL_ERROR, | ||
| ); |
There was a problem hiding this comment.
When GraphQL returns errors, the HTTP status is typically 200/"OK". Passing res.statusText as the CustomError type will therefore set secondaryMessage to "OK", which is not a meaningful secondary message for users. Consider using a stable CustomError type (e.g., CustomError.GRAPHQL_ERROR) and/or mapping from res.data.errors[0].type when available.
| ).toEqual( | ||
| expect.objectContaining({ | ||
| level: "B-", | ||
| percentile: expect.closeTo(65.02918514848255, 10), |
There was a problem hiding this comment.
expect.closeTo(…, 10) uses an extremely tight tolerance (10 decimal digits), which can still cause flaky tests if floating-point behavior changes slightly across Node/Jest versions. Consider using a looser precision (fewer digits) or asserting percentile with toBeCloseTo separately from the level assertion.
| percentile: expect.closeTo(65.02918514848255, 10), | |
| percentile: expect.closeTo(65.02918514848255, 5), |
Summary
fetchin non-test environmentserrorsshapeNODE_ENV=testin pat-info testsContext
This branch contains commit
1c4a4b5from #4714 plus follow-up commit727d211.I could not push to the original PR head branch (
biplavbarua:master) due permissions, so this PR is opened as a replacement path for review.Related: #4714