fix: change fetch-retry import to esm module import#611
Conversation
WalkthroughThe code updates the way the fetch-retry module is imported and initialized, switching from a CommonJS require-based approach to an ES module import using a factory function. This change maintains the same retry functionality for fetch but modernizes the import syntax. Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 669f3c8 in 29 seconds. Click for details.
- Reviewed
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/src/lib/prompts/fetch.ts:3
- Draft comment:
Updated to ESM import. Ensure that your TS config (e.g., esModuleInterop) supports default imports from CommonJS modules. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/traceloop-sdk/src/lib/prompts/fetch.ts:5
- Draft comment:
Conversion to use the factory function with ESM style is correct. This ensures consistent module usage. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_GLMLzVGE2Ap7Lmo8
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/traceloop-sdk/src/lib/prompts/fetch.ts (1)
17-21: UndefinedtraceloopSyncMaxRetriescan bypass your guard
attempt >= traceloopSyncMaxRetries!relies on a non-null assertion, yet
retries: traceloopSyncMaxRetriesmay passundefinedwhen the caller omits
the field, causing:
- The comparison to evaluate to
falsefor all attempts (sinceundefinedis NaN).fetch-retryto use its own default retry count (3), creating a silent behaviour change.Safer pattern:
- const { apiKey, baseUrl, traceloopSyncMaxRetries } = options; + const { apiKey, baseUrl, traceloopSyncMaxRetries = 3 } = options; // default ... - retries: traceloopSyncMaxRetries, + retries: traceloopSyncMaxRetries, ... - if (attempt >= traceloopSyncMaxRetries!) return false; + if (attempt >= traceloopSyncMaxRetries) return false;This removes the non-null assertion and guarantees deterministic retry logic.
🧹 Nitpick comments (1)
packages/traceloop-sdk/src/lib/prompts/fetch.ts (1)
5-10: Rename for clarity
fetchRetrynow holds the wrapped fetch rather than the factory, which can
confuse future readers. Consider:-const fetchRetry = fetchRetryFactory(fetch); +const fetchWithRetry = fetchRetryFactory(fetch);and replace subsequent calls accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/traceloop-sdk/src/lib/prompts/fetch.ts(1 hunks)
🔇 Additional comments (1)
packages/traceloop-sdk/src/lib/prompts/fetch.ts (1)
3-5: Mind the CJS → ESM interop & missing typings
fetch-retryships as a CommonJS export (module.exports = function…).
The default-import syntax will only compile if:
tsconfig.jsonhas"esModuleInterop": trueand- the package provides its own
index.d.ts(it does not at the time of writing).If either pre-condition is false, the build will break with
TS1192: Module 'fetch-retry' has no default export.Recommended, type-safe import that works with or without
esModuleInterop:-import fetchRetryFactory from "fetch-retry"; +// eslint-disable-next-line @typescript-eslint/consistent-type-imports +import fetchRetryFactory = require("fetch-retry"); // CJS default exportAlternatively keep the previous
require("fetch-retry")(fetch)construct.Please verify the compiler options before merging.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed d143c24 in 26 seconds. Click for details.
- Reviewed
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/src/lib/prompts/fetch.ts:3
- Draft comment:
The ESM import of 'fetch-retry' as 'fetchBuilder' is correctly applied. Ensure that the alias matches the package documentation for clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_pyZuVcS9jGhwLfWy
You can customize
2 participants
Important
Refactor
fetch-retryimport infetch.tsto use ES module import withfetchBuilder(fetch).fetch-retryimport infetch.tsfromrequireto ES module import usingfetchBuilder(fetch).This description was created by
for d143c24. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit