chore: drop backoff dep and convert remaining require() calls to ESM#8195
Open
chore: drop backoff dep and convert remaining require() calls to ESM#8195
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR removes the Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/deploy/upload-files.ts (1)
80-127: Consider adding generic typing toretryUploadto preserve response types.The retry attempt semantics are correct: with
maxRetry = n, the function allows up ton + 1total attempts (initial attempt plusnretries), matching the previousfailAfter(maxRetry)behavior.However,
retryUploadcurrently returnsPromise<unknown>, causing theresponsevariable (lines 30 and 42) to lose type information fromapi.uploadDeployFile()andapi.uploadDeployFunction(). Add a generic parameter to preserve the response type:Suggested typing refinement
-const retryUpload = (uploadFn: (retryCount: number) => Promise<unknown>, maxRetry: number) => - new Promise((resolve, reject) => { +const retryUpload = <T>(uploadFn: (retryCount: number) => Promise<T>, maxRetry: number): Promise<T> => + new Promise<T>((resolve, reject) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/deploy/upload-files.ts` around lines 80 - 127, The retryUpload helper should be made generic so it preserves the resolved type from callers: change retryUpload to accept a type parameter (e.g. <T>) and type uploadFn as (retryCount: number) => Promise<T>, and have retryUpload return Promise<T>; keep internal variables (lastError, retryCount, scheduleNextAttempt, tryUpload) the same but ensure resolve is typed to accept T and reject remains unknown/any as before; update any call sites of retryUpload (e.g., uses with api.uploadDeployFile and api.uploadDeployFunction) to infer or pass the concrete type if needed so the response variables keep their original types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/deploy/upload-files.ts`:
- Around line 80-127: The retryUpload helper should be made generic so it
preserves the resolved type from callers: change retryUpload to accept a type
parameter (e.g. <T>) and type uploadFn as (retryCount: number) => Promise<T>,
and have retryUpload return Promise<T>; keep internal variables (lastError,
retryCount, scheduleNextAttempt, tryUpload) the same but ensure resolve is typed
to accept T and reject remains unknown/any as before; update any call sites of
retryUpload (e.g., uses with api.uploadDeployFile and api.uploadDeployFunction)
to infer or pass the concrete type if needed so the response variables keep
their original types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63df52d9-8379-41d4-a471-a979f681d265
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonsrc/commands/database/legacy/utils.tssrc/commands/functions/functions-create.tssrc/commands/functions/functions-invoke.tssrc/utils/deploy/upload-files.tstests/unit/commands/database/legacy/utils.test.tstests/unit/commands/functions/functions-invoke.test.tstests/unit/utils/gh-auth.test.ts
💤 Files with no reviewable changes (1)
- package.json
Continues the Milestone 1 dependency-pruning work from #8189. - Replace the single `backoff` call site in `src/utils/deploy/upload-files.ts` with a small inline Fibonacci retry that preserves the existing delay sequence (5s, 5s, 10s, 15s, 25s, … capped at UPLOAD_MAX_DELAY) and the `failAfter(maxRetry)` semantics. Also migrate `tests/unit/utils/gh-auth.test.ts` off `backoff` to a plain polling loop. - Convert the three remaining `require(path)` call sites to native ESM equivalents: `functions-create.ts` and `database/legacy/utils.ts` now use `JSON.parse(readFileSync(...))` for package.json lookups; `functions-invoke.ts` becomes async and supports both `.json` (readFile) and `.js/.mjs/.cjs` (dynamic `import()`) payloads. - Remove `backoff` and `@types/backoff` from package.json. - Add unit tests for `getPackageJSON` and export + test `processPayloadFromFlag` to lock in the JSON + dynamic-import behavior. Made-with: Cursor
jherr
force-pushed
the
chore/backoff-and-require-cleanup
branch
from
a0d6ee5 to
38ed20d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/unit/commands/database/legacy/utils.test.ts (1)
56-74: Optional: consider covering thenull/ array edge case fordependencieset al.Since
typeof null === 'object'andtypeof [] === 'object', apackage.jsonwith e.g."dependencies": nullor"dependencies": []currently passes validation ingetPackageJSONand is returned as-is. This is a pre-existing behavior in the function (not introduced by this refactor), but given you're adding a fresh test suite it may be worth either pinning that behavior with an explicit test or tightening the check ingetPackageJSONto also rejectnull/arrays. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/commands/database/legacy/utils.test.ts` around lines 56 - 74, getPackageJSON currently treats null and arrays as valid because typeof null/array === 'object'; update the validation inside getPackageJSON to require a plain object for keys dependencies, devDependencies and scripts by checking value !== null && !Array.isArray(value) && typeof value === 'object', and throw the same style of error (e.g., "Expected object at package.json#<key>, got <type>") when the check fails so tests like the ones in utils.test.ts will also fail for null/arrays; adjust or add tests to assert that null and arrays are rejected for these keys if you prefer tightening behavior.src/commands/functions/functions-invoke.ts (1)
75-77: Prefer async file read inside async function.Now that
processPayloadFromFlagisasync, consider usingfs.promises.readFileto avoid blocking the event loop on disk I/O.♻️ Proposed refactor
-import fs from 'fs' +import fs from 'fs' +import { readFile } from 'fs/promises'if (payloadpath.endsWith('.json')) { - return JSON.parse(fs.readFileSync(payloadpath, 'utf8')) + return JSON.parse(await readFile(payloadpath, 'utf8')) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/functions/functions-invoke.ts` around lines 75 - 77, The sync file read blocks the event loop inside the async function processPayloadFromFlag; replace fs.readFileSync usage when handling payloadpath.endsWith('.json') with an awaited async read from fs.promises.readFile, then JSON.parse the resulting string and return it. Update references to payloadpath and JSON.parse in processPayloadFromFlag to use: const contents = await fs.promises.readFile(payloadpath, 'utf8'); return JSON.parse(contents); and ensure errors propagate or are caught where processPayloadFromFlag is invoked.src/utils/deploy/upload-files.ts (1)
82-122: Optional: drop the redundantlastErrorbinding.
lastErroris only ever assignederrorwithin the samecatchblock before being used inreject(lastError)on line 115. You can just reject witherrorand remove the outer binding to simplify the closure.♻️ Proposed simplification
let lastError: unknown let retryCount = 0 let previousDelay = 0 let nextDelay = UPLOAD_INITIAL_DELAY @@ - } catch (error) { - lastError = error - const status = (error as { status?: number } | null)?.status - const name = (error as { name?: string } | null)?.name + } catch (error) { + const { status, name } = (error ?? {}) as { status?: number; name?: string } if (status === 400 || status === 422) { reject(error) return } if ((typeof status === 'number' && status > 400) || name === 'FetchError') { retryCount += 1 if (retryCount > maxRetry) { - reject(lastError) + reject(error) return }And drop the
let lastError: unknowndeclaration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/deploy/upload-files.ts` around lines 82 - 122, Remove the redundant outer binding lastError and use the caught error directly: in the catch block inside tryUpload (which calls uploadFn), delete the let lastError: unknown declaration and the assignment lastError = error, then replace reject(lastError) (on the max-retry path) with reject(error); keep the rest of the retry logic (retryCount, maxRetry, scheduleNextAttempt, status/name checks) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/commands/functions/functions-invoke.ts`:
- Around line 75-77: The sync file read blocks the event loop inside the async
function processPayloadFromFlag; replace fs.readFileSync usage when handling
payloadpath.endsWith('.json') with an awaited async read from
fs.promises.readFile, then JSON.parse the resulting string and return it. Update
references to payloadpath and JSON.parse in processPayloadFromFlag to use: const
contents = await fs.promises.readFile(payloadpath, 'utf8'); return
JSON.parse(contents); and ensure errors propagate or are caught where
processPayloadFromFlag is invoked.
In `@src/utils/deploy/upload-files.ts`:
- Around line 82-122: Remove the redundant outer binding lastError and use the
caught error directly: in the catch block inside tryUpload (which calls
uploadFn), delete the let lastError: unknown declaration and the assignment
lastError = error, then replace reject(lastError) (on the max-retry path) with
reject(error); keep the rest of the retry logic (retryCount, maxRetry,
scheduleNextAttempt, status/name checks) unchanged.
In `@tests/unit/commands/database/legacy/utils.test.ts`:
- Around line 56-74: getPackageJSON currently treats null and arrays as valid
because typeof null/array === 'object'; update the validation inside
getPackageJSON to require a plain object for keys dependencies, devDependencies
and scripts by checking value !== null && !Array.isArray(value) && typeof value
=== 'object', and throw the same style of error (e.g., "Expected object at
package.json#<key>, got <type>") when the check fails so tests like the ones in
utils.test.ts will also fail for null/arrays; adjust or add tests to assert that
null and arrays are rejected for these keys if you prefer tightening behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10007a25-ea27-42f1-9fe7-ce21729a6a6b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonsrc/commands/database/legacy/utils.tssrc/commands/functions/functions-create.tssrc/commands/functions/functions-invoke.tssrc/utils/deploy/upload-files.tstests/unit/commands/database/legacy/utils.test.tstests/unit/commands/functions/functions-invoke.test.tstests/unit/utils/gh-auth.test.ts
💤 Files with no reviewable changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/commands/functions/functions-create.ts
- tests/unit/commands/functions/functions-invoke.test.ts
- tests/unit/utils/gh-auth.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Continues the Milestone 1 dependency-pruning work from #8189 by finishing items B and C from the follow-up list:
backoff/@types/backoff. Its only runtime use was insrc/utils/deploy/upload-files.ts; replaced with a small inline Fibonacci retry helper that preserves the existing delay sequence (5s, 5s, 10s, 15s, 25s, …capped atUPLOAD_MAX_DELAY), the0.5jitter factor, and thefailAfter(maxRetry)"6 attempts when maxRetry=5" semantics.tests/unit/utils/gh-auth.test.tsmigrated offbackoffto a plain polling loop.require()calls to ESM.src/commands/functions/functions-create.ts(x2):require(packageJson)→JSON.parse(fs.readFileSync(...))via a smallreadJsonFilehelper.src/commands/database/legacy/utils.ts:createRequire(join(dir, 'package.json'))('./package.json')→JSON.parse(fs.readFileSync(join(dir, 'package.json'), 'utf8')).src/commands/functions/functions-invoke.ts:processPayloadFromFlagbecomes async;.jsonpayloads usereadFile+JSON.parse, other files go through dynamicimport()(viapathToFileURL) withimported.default ?? importedto preserve the CJS interop the oldrequire()provided.tests/unit/commands/database/legacy/utils.test.ts(8 cases) — exercisesgetPackageJSONhappy path plus each of the shape-validation errors.tests/unit/commands/functions/functions-invoke.test.ts(7 cases) — exercisesprocessPayloadFromFlagwith inline JSON strings,.json/.mjs/.cjsfile payloads, missing paths, and load-time errors (processPayloadFromFlagis now exported for testability).Test plan
npm run typechecknpm run lintnpm exec vitest -- run tests/unit/utils/deploy/upload-files.test.ts(2/2 — retry-count-on-retries and no-retry-on-400 still pass against the inline Fibonacci implementation)npm exec vitest -- run tests/unit/utils/gh-auth.test.ts(1/1)npm exec vitest -- run tests/unit/commands/database/legacy/utils.test.ts(8/8, new)npm exec vitest -- run tests/unit/commands/functions/functions-invoke.test.ts(7/7, new)npm run test:unit— 337 passed. The 5 failures are pre-existing onmain(copy-template-dir.test.ts× 4 hit a Node 25fs.rmdirSyncdeprecation;generate-autocompletion.test.ts× 1 has a stale snapshot).npm exec vitest -- run tests/integration/commands/functions-invoke/functions-invoke.test.ts(5/5 — validates the now-asyncprocessPayloadFromFlagpath end-to-end)Made with Cursor