feat: add proxy support with node-fetch-native/proxy
#40
Conversation
🦋 Changeset detectedLatest commit: 063d291 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update modifies package dependencies and refines HTTP request handling in the project. The root Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant translate.ts
participant node-fetch-native/proxy
participant xfetch
Caller->>translate.ts: makeRequest(postData, proxyUrl, dlSession)
translate.ts->>node-fetch-native/proxy: createProxy({ url: proxyUrl })
translate.ts->>xfetch: xfetch(url, { ...options, ...proxyOptions })
xfetch-->>translate.ts: HTTP response
translate.ts-->>Caller: Response/result
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
yarn install v1.22.22 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds proxy support by integrating node-fetch-native/proxy and updates related components to align with the new proxy functionality.
- Updates translate.ts to use xfetch and apply proxy settings via createProxy.
- Adjusts constants.ts to type COMMON_HEADERS with a proper type assertion.
- Cleans up the CLI fetch initialization by removing obsolete ts-expect-error directives.
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/@deeplx/core/src/translate.ts | Replaces fetchApi with xfetch and integrates createProxy for proxy support. |
| packages/@deeplx/core/src/constants.ts | Updates COMMON_HEADERS to use type assertion with satisfies HeadersInit. |
| packages/@deeplx/cli/src/fetch.ts | Removes ts-expect-error comments for global fetch and Headers assignment. |
Files not reviewed (2)
- package.json: Language not supported
- packages/@deeplx/core/package.json: Language not supported
Comments suppressed due to low confidence (2)
packages/@deeplx/core/src/translate.ts:33
- If proxy support is now implemented via createProxy, please update or remove the 'unsupported yet' comment to accurately reflect the current status.
proxyUrl?: string, // unsupported yet
packages/@deeplx/cli/src/fetch.ts:3
- Confirm that the removal of @ts-expect-error comments does not reintroduce type compatibility issues between node-fetch and the global fetch assignment.
globalThis.fetch ??= fetch
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #40 +/- ##
=======================================
Coverage 86.55% 86.55%
=======================================
Files 4 4
Lines 119 119
Branches 29 30 +1
=======================================
Hits 103 103
Misses 15 15
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
commit: |
size-limit report 📦
|
|
Deploy preview for deeplx ready! ✅ Preview Built with commit 063d291. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 9a550abf4f1cabae6fc1e0ab97a761c9c4d2160b in 1 minute and 41 seconds
More details
- Looked at
110lines of code in5files - Skipped
1files when reviewing. - Skipped posting
15drafted comments based on config settings.
1. package.json:56
- Draft comment:
Using npm aliases in the resolutions (node-fetch, undici, undici-types) is acceptable, but double-check that these overrides don't conflict with any dependency versions elsewhere in the project. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. package.json:35
- Draft comment:
Removal of '@types/web' may be intentional; verify that no code elsewhere depends on these types. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. packages/@deeplx/cli/src/fetch.ts:4
- Draft comment:
Assignment to globalThis.fetch and globalThis.Headers looks fine now, but ensure the types align with environment expectations since the ts-expect-error comments were removed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. packages/@deeplx/core/src/translate.ts:1
- Draft comment:
Importing and using createProxy from 'node-fetch-native/proxy' is a neat addition. Verify that createProxy gracefully handles an undefined proxyUrl. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to verify the behavior of a function when given an undefined input. This falls under asking the author to ensure the behavior is intended, which is against the rules.
5. packages/@deeplx/core/src/translate.ts:3
- Draft comment:
Switching from fetchApi to xfetch should be reviewed to ensure behavior consistency. Confirm that xfetch’s error handling and request options align with previous expectations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. package.json:35
- Draft comment:
Removed unused '@types/web' dependency. Confirm it isn’t required by any part of the code. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. package.json:56
- Draft comment:
Using resolutions for 'node-fetch', 'undici', and 'undici-types' is intentional. Verify that enforcing these versions doesn’t conflict with any dependent packages. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. packages/@deeplx/cli/src/fetch.ts:4
- Draft comment:
Removed obsolete '@ts-expect-error' comments. Confirm that globalThis.fetch and globalThis.Headers now have compatible types. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. packages/@deeplx/core/package.json:48
- Draft comment:
Added the 'node-fetch-native' dependency for proxy support. Ensure its version remains compatible with the resolutions enforced in the root package.json. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
10. packages/@deeplx/core/src/constants.ts:79
- Draft comment:
Using 'as const satisfies HeadersInit' improves type safety for COMMON_HEADERS. Verify that consumers of COMMON_HEADERS don’t require mutable headers. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
11. packages/@deeplx/core/src/translate.ts:1
- Draft comment:
Importing 'createProxy' enables proxy support—ensure that createProxy handles an undefined proxyUrl gracefully. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment is speculative - it's asking to verify behavior rather than pointing out a definite issue. We don't have evidence that createProxy doesn't handle undefined gracefully. The library node-fetch-native/proxy is a third-party dependency that we should assume works correctly. The code already properly handles proxyUrl as optional.
The comment could be valid if there's evidence that createProxy throws errors with undefined urls. The proxy handling could be important for reliability.
Without concrete evidence of an issue, this is just speculative. The comment is asking for verification rather than pointing out a clear problem that needs fixing.
Delete this comment as it's speculative and asks for verification without evidence of an actual issue. We should assume third-party libraries work correctly unless proven otherwise.
12. packages/@deeplx/core/src/translate.ts:3
- Draft comment:
Switched from 'fetchApi' to 'xfetch'. Ensure that 'xfetch' supports all required options, especially proxy configurations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
13. packages/@deeplx/core/src/translate.ts:42
- Draft comment:
Consider conditionally including the proxy options only when proxyUrl is provided. Ensure createProxy returns a no-op or empty object if proxyUrl is undefined. - Reason this comment was not posted:
Comment looked like it was already resolved.
14. packages/@deeplx/core/src/translate.ts:33
- Draft comment:
Update the comment for the proxyUrl parameter—remove misleading 'unsupported yet' text now that proxy support is implemented. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
15. package.json:2
- Draft comment:
The package name is set to '@unts/deeplx', but the repository URL uses 'un-ts/deeplx.git'. It seems there is an inconsistency (possibly a missing hyphen) between the package name and the repository owner. Please verify if this is intentional or a typo. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_FVdGA0Fi2BqIyf4F
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![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
🧹 Nitpick comments (1)
packages/@deeplx/core/src/translate.ts (1)
33-33: Changed parameter name to reflect actual usage.Renaming from
_proxyUrltoproxyUrlindicates this parameter is now actively used rather than just reserved for future implementation. However, the comment "unsupported yet" should be removed since the feature is now supported.- proxyUrl?: string, // unsupported yet + proxyUrl?: string,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 4b07662 and 9a550abf4f1cabae6fc1e0ab97a761c9c4d2160b.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.json(1 hunks)packages/@deeplx/cli/src/fetch.ts(0 hunks)packages/@deeplx/core/package.json(1 hunks)packages/@deeplx/core/src/constants.ts(3 hunks)packages/@deeplx/core/src/translate.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/@deeplx/cli/src/fetch.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/@deeplx/core/src/translate.ts (3)
packages/@deeplx/core/src/types.ts (1)
TranslationResponse(75-85)packages/@deeplx/core/src/constants.ts (2)
API_URL(60-60)COMMON_HEADERS(69-82)packages/@deeplx/core/src/utils.ts (1)
formatPostString(31-40)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
packages/@deeplx/core/package.json (1)
51-51: LGTM: Added dependency for proxy support.The addition of
node-fetch-nativeis appropriate for implementing the proxy support mentioned in the PR title.package.json (2)
53-53: LGTM: Updated dependency version.Updating
yarn-berry-deduplicateto the latest version is a good maintenance practice.
56-59: LGTM: Added appropriate dependency resolutions.The added resolutions ensure consistent fetch implementations across the project:
- Redirecting
node-fetchtonode-fetch-nativealigns with the new proxy support feature- Standardizing
undiciandundici-typesversions prevents potential compatibility issuesThese changes follow good package management practices.
packages/@deeplx/core/src/constants.ts (2)
1-1: LGTM: Added import for improved type safety.Importing
HeadersInitfrom 'undici' provides proper typing for the headers object.
69-82: LGTM: Improved type safety with const assertion.Using
as const satisfies HeadersInitprovides two benefits:
as constmakes the object immutable with literal typessatisfies HeadersInitensures the object conforms to the expected interfaceThis is a TypeScript best practice for constants that need to maintain both their literal types and interface compatibility.
packages/@deeplx/core/src/translate.ts (2)
1-3: LGTM: Updated imports to support proxy functionality.The changes appropriately:
- Import
createProxyfrom 'node-fetch-native/proxy' to enable proxy support- Replace
fetchApiwithxfetchfor a more consistent fetch implementation
36-43: LGTM: Implemented proxy support.The implementation correctly:
- Switches from
fetchApitoxfetch- Uses
createProxy({ url: proxyUrl })to enable proxy configuration when specifiedThis allows the function to work through a proxy when needed, which is the main feature of this PR.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 25d94af in 1 minute and 37 seconds
More details
- Looked at
124lines of code in6files - Skipped
1files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. packages/@deeplx/core/src/translate.ts:1
- Draft comment:
Importing createProxy is good. Consider verifying that createProxy handles undefined or empty proxyUrl gracefully. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. packages/@deeplx/core/src/translate.ts:42
- Draft comment:
Spreading createProxy output is concise; consider validating proxyUrl before passing to createProxy if needed. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. packages/@deeplx/cli/src/fetch.ts:1
- Draft comment:
Switch to node-fetch-native import looks correct; ensure global fetch/Headers override works in all environments. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
4. package.json:55
- Draft comment:
Review resolutions: Ensure node-fetch-native and undici versions are fully compatible with all dependent packages. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure compatibility of dependencies, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.
5. packages/@deeplx/cli/src/fetch.ts:3
- Draft comment:
If type issues are resolved with node-fetch-native, consider removing the eslint-disable comments for global assignments. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. packages/@deeplx/core/src/constants.ts:69
- Draft comment:
Ensure 'as const satisfies HeadersInit' is supported in your TS config if targeting older TypeScript versions. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. packages/@deeplx/core/src/translate.ts:33
- Draft comment:
Update the proxyUrl parameter comment to reflect that proxy support is now active (remove 'unsupported yet'). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment is correct - the code now clearly uses the proxyUrl parameter with createProxy, so the comment saying it's "unsupported yet" is outdated and incorrect. However, this is just a documentation update, not a code quality or logic issue. Our rules say to only comment if there's clearly a code change required.
Documentation accuracy is important for maintainability. Outdated comments can mislead developers.
While accurate documentation is valuable, our explicit rules state to only comment when code changes are required. Documentation-only changes are not in scope.
Delete the comment since it only suggests a documentation update rather than a required code change.
8. packages/@deeplx/core/src/translate.ts:42
- Draft comment:
Consider validating proxyUrl before spreading proxy settings to avoid issues when proxyUrl is undefined or empty. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The comment makes a valid point about defensive programming. However, looking at the code, createProxy() is imported from node-fetch-native/proxy, which is a third-party library. Without knowing how that library handles undefined/empty URLs, we can't be certain there's actually a problem. The library may already handle this case gracefully.
I may be too quick to dismiss potential runtime issues. An undefined proxyUrl could cause problems if the library doesn't handle it well.
While the concern is valid, we don't have enough context about the createProxy function's behavior to be certain this is an issue. The library may already handle this case appropriately.
Delete the comment since we don't have strong evidence that this is actually a problem without knowing how createProxy handles undefined values.
9. packages/@deeplx/core/src/translate.ts:36
- Draft comment:
Verify that xfetch provides behavior equivalent to the previous fetchApi, especially regarding custom headers and proxy support. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify the behavior ofxfetchcompared tofetchApi, which violates the rule against asking the author to confirm or verify behavior. It does not provide a specific suggestion or point out a specific issue.
10. package.json:2
- Draft comment:
It looks like the package is named "@unts/deeplx", but given that the repository owner is "un-ts", please confirm if this naming is intentional or if it should be made consistent. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. package.json:6
- Draft comment:
The author's name is listed as "JounQin". Please verify if this is the intended spelling or if it should be corrected (e.g., "JonQin"). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_8AzAnLIuRsbhiWN5
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
|
Important
Add proxy support using
node-fetch-native/proxyand update dependencies tonode-fetch-native.createProxyfromnode-fetch-native/proxyintranslate.ts.node-fetchwithnode-fetch-nativeincli/package.jsonandcore/package.json.package.jsonresolutions to usenode-fetch-nativeandundici.constants.tsby usingas const satisfies HeadersInitforCOMMON_HEADERS._proxyUrltoproxyUrlinmakeRequestfunction intranslate.tsfor consistency.This description was created by
for 25d94af. It will automatically update as commits are pushed.
Summary by CodeRabbit