-
Notifications
You must be signed in to change notification settings - Fork 9
Main to origin #90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Main to origin #90
Conversation
Fixes: Get rid of deprecated Node.js API: url.parse #20
Feat/url pool
Revert "Feat/url pool"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @hcfw007, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request updates the file-box package to version 1.7.16 and introduces several enhancements and fixes. Key changes include the addition of environment variables for configuring HTTP request and response timeouts, disabling slice downloading, and setting HTTP chunk size. The update also includes a fix for handling errors during stream writing and improvements to HTTP header processing and chunk downloading. Additionally, the package name has been changed to @juzi/file-box.
Highlights
- Environment Variables Configuration: Introduces environment variables
FILEBOX_HTTP_REQUEST_TIMEOUT,FILEBOX_HTTP_RESPONSE_TIMEOUT,NO_SLICE_DOWN, andHTTP_CHUNK_SIZEto allow users to configure HTTP request timeouts, disable slice downloading, and set HTTP chunk size. - Error Handling Improvement: Fixes an issue where errors during stream writing were not properly handled, ensuring that the write stream is closed before rejecting the promise.
- HTTP Header Processing Enhancement: Improves HTTP header processing by including headers and proxy URL when making HTTP HEAD requests, allowing for more flexible and configurable requests.
- Chunk Downloading Enhancement: Adds chunk downloading functionality to download files in multiple parts, improving reliability and supporting breakpoint re-downloading.
- Package Name Change: Changes the package name from
file-boxto@juzi/file-box.
Changelog
Click here to see the changelog
- .vscode/settings.json
- Removed the file.
- README.md
- Added documentation for the new environment variables.
- Updated the history section to reflect the changes in version 1.7.
- package.json
- Updated the package name to
@juzi/file-box. - Updated the package version to 1.7.16.
- Added new dependencies and devDependencies, including
@types/node,@typescript-eslint/eslint-plugin,eslint-plugin-n,https-proxy-agent,tap, andtypescript. - Modified the test script to include a timeout.
- Updated the package name to
- src/config.ts
- Added constants for HTTP request timeout, HTTP response timeout, disabling slice downloading, HTTP chunk size, and ready retry, configurable via environment variables.
- src/file-box.ts
- Added
READY_RETRYimport from./config.js. - Added
headersproperty to the FileBox constructor. - Added
urlgetter to return the remote URL. - Added
proxyUrlproperty anduseProxyUrlmethod to set a proxy URL. - Modified the
readymethod to retry on failure. - Modified the
_syncUrlMetadatamethod to include headers and proxy URL when making HTTP HEAD requests. - Modified the
_transformUrlToStreammethod to include headers and proxy URL when creating the HTTP stream. - Modified the pipe method to improve error handling.
- Added error handling to the write stream in
pipemethod.
- Added
- src/misc.spec.ts
- Added a test case for
httpStreamto verify chunk downloading.
- Added a test case for
- src/misc.ts
- Added imports for various modules, including
assert,crypto,events,fs,http,https,os,path,stream, andurl. - Added constants for HTTP chunk size, HTTP request timeout, HTTP response timeout, and disabling slice downloading.
- Modified the
httpHeadHeaderfunction to include headers and proxy URL when making HTTP HEAD requests. - Modified the
httpStreamfunction to support chunk downloading and proxy URLs. - Added functions for downloading files in chunks, setting proxy options, and converting streams to buffers.
- Refactored
httpStreamto usefetchanddownloadFileInChunksfor improved reliability and flexibility.
- Added imports for various modules, including
- src/mod.ts
- Exported all members from './misc.js'.
- src/pure-functions/sized-chunk-transformer.ts
- Corrected a typo in Buffer.concat.
- src/urn-registry/uniform-resource-name-registry.ts
- Corrected typos in array destructuring.
- src/version.ts
- Removed an extra blank line.
- tests/fixtures/smoke-testing.ts
- Updated the import path for
file-boxto@juzi/file-box.
- Updated the import path for
- tests/network-timeout.spec.ts
- Added a new test file to verify network timeout functionality.
- tsconfig.json
- Set
noUnusedLocalsto false.
- Set
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A file in a box,
Across the network it flies,
Data finds its home.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request introduces several updates and enhancements to the file-box library, including environment variable configurations, improved HTTP handling with proxy support and chunk downloading, and dependency updates. The changes seem well-structured and address potential network timeout issues. However, some areas require closer examination and potential refinement.
Summary of Findings
- Error Handling in Chunk Downloading: The chunk downloading logic includes retry mechanisms, but the error handling could be more robust to ensure temporary file cleanup and consistent error reporting.
- Redundant Protocol Check: The protocol check in
httpStreammight be redundant as it's already performed infetch. Consider removing the check inhttpStreamto simplify the logic. - Inconsistent Naming: The naming of the environment variable
FILE_BOX_READY_RETRYinsrc/config.tsand its usage insrc/file-box.tsis inconsistent. Standardize the naming for clarity.
Merge Readiness
The pull request includes significant improvements and new features. However, the identified issues related to error handling, redundant checks, and naming inconsistencies should be addressed before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. Addressing the critical and high severity comments is highly recommended before merging.
| void rm(tmpFile, { force: true }) | ||
| writeStream.close() | ||
| throw new Error(`Download file with chunk failed! ${err.message}`, { cause: err }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The temporary file is removed only when the retry count exceeds the limit. If an error occurs during chunk download but retries are still available, the temporary file should be cleaned up to avoid accumulating orphaned files. Additionally, consider logging the error that caused the retry.
const err = error as Error
console.error(`Download file with chunk failed (attempt ${retries}): ${err.message}`)
if (--retries <= 0) {
void rm(tmpFile, { force: true })
writeStream.close()
throw new Error(`Download file with chunk failed! ${err.message}`, { cause: err })| export const HTTP_CHUNK_SIZE = Number(process.env['FILEBOX_HTTP_CHUNK_SIZE']) | ||
| || 1024 * 512 | ||
|
|
||
| export const READY_RETRY = Number(process.env['FILE_BOX_READY_RETRY']) || 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The environment variable name FILE_BOX_READY_RETRY is inconsistent with other variable names (using underscores instead of camel case). Consider renaming it to FILEBOX_READY_RETRY to maintain consistency.
| export const READY_RETRY = Number(process.env['FILE_BOX_READY_RETRY']) || 3 | |
| export const READY_RETRY = Number(process.env['FILEBOX_READY_RETRY']) || 3 |
| tryCount ++ | ||
| if (tryCount >= READY_RETRY) { | ||
| throw e | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The retry mechanism is implemented, but consider adding a delay between retries to avoid overwhelming the server. Also, logging the error before retrying would be helpful for debugging.
} catch (e) {
tryCount ++
console.error(`FileBox: ready() failed (attempt ${tryCount}/${READY_RETRY}): ${e}`)
await new Promise(resolve => setTimeout(resolve, 1000)) // Add a 1-second delay
if (tryCount >= READY_RETRY) {
throw e
}| md5: obj.md5, | ||
| name: obj.name, | ||
| size: obj.size, | ||
| headers: obj.headers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
This is a huge PR, and it’s an anti-pattern. I’d really appreciate it if you could follow the same collaboration best practices we use in the Wechaty community: create an issue first, break down the new features into the smallest possible pieces, describe each feature in its own issue, and open a separate PR for each one. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR moves merging activity to a dedicated main-to-origin branch to avoid inadvertently updating the main branch in the juzibot repository, while also introducing several refactorings and enhancements.
- Updated network timeout tests and smoke testing fixtures
- Significant refactoring in network handling (src/misc.ts) including support for proxy settings and chunked downloads
- Documentation, configuration, and workflow adjustments to reflect these changes
Reviewed Changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/network-timeout.spec.ts | Added tests for HTTP request/response timeout scenarios with sinon fake timers |
| tests/fixtures/smoke-testing.ts | Adjusted import to use the new module location |
| src/version.ts | Minor cleanup related to version management |
| src/mod.ts | Exported misc module to surface additional utility functions |
| src/misc.ts | Refactored HTTP streaming, added proxy support, and introduced chunked download logic |
| src/misc.spec.ts | Added test verifying HTTP streaming in chunked mode |
| src/file-box.ts | Enhanced proxy support and ready method retry logic |
| src/config.ts | Updated environment variable definitions and defaults |
| README.md | Updated documentation regarding environment variables and deprecations |
| .github/workflows/npm.yml | Updated NPM publishing workflow token |
Files not reviewed (2)
- .vscode/settings.json: Language not supported
- package.json: Language not supported
Comments suppressed due to low confidence (3)
src/misc.ts:50
- [nitpick] The error message uses the already decremented 'REDIRECT_TTL' value, which may be confusing. Consider storing the original TTL value and reflecting that in the error message.
throw new Error(`ttl expired! too many(>${REDIRECT_TTL}) 302 redirection.`)
src/config.ts:15
- [nitpick] The environment variable 'FILE_BOX_READY_RETRY' is inconsistent with the naming convention used for other variables (e.g. 'FILEBOX_HTTP_REQUEST_TIMEOUT'). Consider renaming it for consistency.
export const READY_RETRY = Number(process.env['FILE_BOX_READY_RETRY']) || 3
src/misc.ts:125
- Consider propagating the error from the request's error event instead of simply destroying the request to avoid losing important error details.
.on('error', () => { req.destroy() })
重新开了一个PR,因为在 juzibot 的 repo 上开了一个 main-to-origin 分支专门用于合并,以避免解决冲突时意外 update 了 juzibot 的 main 分支。