feat(node-fetch): improve file access #2009
Merged
Conversation
This file contains 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
✅
|
✅
|
✅
|
✅
|
✅
|
|
What do you think @coderabbitai |
|
@ardatan The changes look great! I particularly like:
The implementation is clean and achieves the PR objectives perfectly. I recommend merging these changes. 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 4236 ✅ Actions performedComments resolved. |
WalkthroughThis pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant F as getResponseForFile
participant A as fsPromises.access
participant S as fsPromises.stat
participant R as Response
C->>F: Request file URL
F->>A: Check file access
A-->>F: Access result (success/error)
alt File Not Found
F-->>C: Return 404 Response ("Not Found")
else Permission Denied
F-->>C: Return 403 Response ("Forbidden")
else File Accessible
F->>S: Retrieve file stats
S-->>F: Return file statistics
F->>R: Create stream and 200 Response with headers
R-->>C: Return 200 Response ("OK")
end
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
packages/node-fetch/tests/non-http-fetch.spec.tsOops! Something went wrong! :( ESLint: 9.18.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![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 (2)
packages/node-fetch/tests/non-http-fetch.spec.ts (1)
24-34: Consider improving cleanup in the forbidden file test.While the test correctly verifies the 403 status code, the cleanup could be more robust.
Consider wrapping the file operations in a try-catch:
const tmpDir = tmpdir(); const path = join(tmpDir, 'forbidden.json'); -writeFileSync(path, '{ "test": 1 }', { mode: 0o000 }); -try { - const response = await fetchPonyfill(pathToFileURL(path)); - expect(response.status).toBe(403); -} finally { - unlinkSync(path); -} +try { + writeFileSync(path, '{ "test": 1 }', { mode: 0o000 }); + try { + const response = await fetchPonyfill(pathToFileURL(path)); + expect(response.status).toBe(403); + } finally { + try { + unlinkSync(path); + } catch (err) { + // Ignore cleanup errors + } + } +} catch (err) { + throw new Error(`Failed to set up test: ${err.message}`); +}packages/node-fetch/src/fetch.ts (1)
13-43: Consider improving content-type detection.While the error handling is excellent, the content-type header could be more accurate.
Consider using a content-type detection library:
+import mime from 'mime-types'; async function getResponseForFile(url: string) { const path = fileURLToPath(url); try { await fsPromises.access(path, fsPromises.constants.R_OK); const stats = await fsPromises.stat(path, { bigint: true, }); const readable = createReadStream(path); return new PonyfillResponse(readable, { status: 200, statusText: 'OK', headers: { - 'content-type': 'application/octet-stream', + 'content-type': mime.lookup(path) || 'application/octet-stream', 'last-modified': stats.mtime.toUTCString(), }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/tough-moles-arrive.md(1 hunks)packages/node-fetch/src/fetch.ts(3 hunks)packages/node-fetch/tests/non-http-fetch.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: unit / deno
- GitHub Check: unit / bun
- GitHub Check: esm
- GitHub Check: unit / node 23
- GitHub Check: alpha / snapshot
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: server (undici)
- GitHub Check: unit / node 18
- GitHub Check: server (ponyfill)
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: type check
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: server (native)
- GitHub Check: e2e / azure-function
- GitHub Check: lint
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: e2e / aws-lambda
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/node-fetch/tests/non-http-fetch.spec.ts (2)
9-17: LGTM! Well-structured test for successful file read.The test case properly verifies both the status code and the response body.
18-23: LGTM! Good test for missing file scenario.The test case correctly verifies the 404 status code for non-existent files.
packages/node-fetch/src/fetch.ts (1)
100-102: LGTM! Proper async handling.The removal of
fakePromiseand direct return of the async response is correct..changeset/tough-moles-arrive.md (1)
1-7: LGTM! Clear and concise changeset.The changeset accurately documents the changes and their impact on the package.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/node-fetch/tests/non-http-fetch.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: unit / deno
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / bun
- GitHub Check: unit / node 20
- GitHub Check: alpha / snapshot
- GitHub Check: unit / node 18
- GitHub Check: server (undici)
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: server (ponyfill)
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: e2e / azure-function
- GitHub Check: server (native)
- GitHub Check: e2e / aws-lambda
- GitHub Check: lint
- GitHub Check: node-fetch (noConsumeBody)
🔇 Additional comments (1)
packages/node-fetch/tests/non-http-fetch.spec.ts (1)
8-21: LGTM! Well-structured test cases for successful read and missing file scenarios.The test cases effectively verify:
- Successful file read with 200 status
- Missing file with 404 status
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When
fetch('file:///...')is used to read files;Summary by CodeRabbit