refactor(deps): reduce direct dependency surface - #161
Conversation
Replace temp-dir, ip, rimraf, and shell helpers with Node APIs. Remove unused download and trigger-template code, declare runtime dependencies explicitly, and add Node 16 type checking.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds CI type checking, replaces temporary-directory and IP dependencies with shared utilities, removes obsolete utilities and trigger templates, updates Jest and npm configuration, and standardizes Chalk imports. ChangesUtility and tooling cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
__tests__/ut/local/impl/baseLocal_test.ts (1)
35-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the migrated temporary-directory paths.
The suite mocks
getTempDir(), but it does not callgetCodeUri()ortryUnzip(). Add archive extraction tests that assert both paths use the mocked temporary directory and UUID suffix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/ut/local/impl/baseLocal_test.ts` around lines 35 - 36, Extend the tests around the existing getTempDir mock to invoke getCodeUri() and tryUnzip() with archive inputs, covering both temporary-directory paths. Assert each generated path uses the mocked /tmp directory and includes the expected UUID suffix, while preserving the existing test setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/subCommands/local/impl/baseLocal.ts`:
- Line 471: Update the cleanup call in the after() method to configure fs.rmSync
with recursive and force enabled plus a positive maxRetries value, ensuring
transient EBUSY, EPERM, and ENOTEMPTY errors are retried without changing the
existing target path.
---
Nitpick comments:
In `@__tests__/ut/local/impl/baseLocal_test.ts`:
- Around line 35-36: Extend the tests around the existing getTempDir mock to
invoke getCodeUri() and tryUnzip() with archive inputs, covering both
temporary-directory paths. Assert each generated path uses the mocked /tmp
directory and includes the expected UUID suffix, while preserving the existing
test setup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f5ddff9-57f1-466a-9d12-8bdf4d396813
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.github/workflows/ci_node16.yaml__tests__/ut/core/downloadFile_test.ts__tests__/ut/local/impl/baseLocal_test.ts__tests__/ut/local/impl/utils_test.ts__tests__/ut/utils/utils_functions_test.ts__tests__/ut/utils/utils_test.tsdocs/architecture.mddocs/testing-plan.mdjestconfig.jsonpackage.jsonpatches/ali-oss+6.18.1.patchsrc/subCommands/build/impl/defaultBuilder.tssrc/subCommands/build/index.tssrc/subCommands/deploy/impl/function.tssrc/subCommands/invoke/index.tssrc/subCommands/local/impl/baseLocal.tssrc/subCommands/local/impl/invoke/phpLocalInvoke.tssrc/subCommands/local/impl/start/phpLocalStart.tssrc/subCommands/local/impl/utils.tssrc/subCommands/model/index.tssrc/subCommands/trigger-template/event-template/cdn-CachedObjectsRefreshed.jsonsrc/subCommands/trigger-template/event-template/cdn-CdnDomainAdded.jsonsrc/subCommands/trigger-template/event-template/cdn-CdnDomainStarted.jsonsrc/subCommands/trigger-template/event-template/cdn-LogFileCreated.jsonsrc/subCommands/trigger-template/event-template/http.jsonsrc/subCommands/trigger-template/event-template/mns-stream.jsonsrc/subCommands/trigger-template/event-template/mns-with-MessageAttributes.jsonsrc/subCommands/trigger-template/event-template/mns-without-MessageAttributes.jsonsrc/subCommands/trigger-template/event-template/oss.jsonsrc/subCommands/trigger-template/event-template/sls.jsonsrc/subCommands/trigger-template/event-template/tablestore.jsonsrc/subCommands/trigger-template/index.tssrc/utils/index.tssrc/utils/verify.ts
💤 Files with no reviewable changes (17)
- src/subCommands/trigger-template/event-template/cdn-CdnDomainStarted.json
- src/subCommands/trigger-template/event-template/tablestore.json
- src/subCommands/trigger-template/event-template/sls.json
- src/subCommands/trigger-template/event-template/mns-stream.json
- src/subCommands/trigger-template/event-template/mns-with-MessageAttributes.json
- src/subCommands/trigger-template/event-template/cdn-CachedObjectsRefreshed.json
- tests/ut/core/downloadFile_test.ts
- src/subCommands/trigger-template/event-template/http.json
- src/subCommands/trigger-template/index.ts
- src/subCommands/trigger-template/event-template/mns-without-MessageAttributes.json
- src/subCommands/trigger-template/event-template/cdn-LogFileCreated.json
- src/subCommands/trigger-template/event-template/oss.json
- patches/ali-oss+6.18.1.patch
- src/subCommands/trigger-template/event-template/cdn-CdnDomainAdded.json
- docs/testing-plan.md
- src/subCommands/local/impl/utils.ts
- tests/ut/utils/utils_test.ts
| logger.debug('after ...'); | ||
| if (this.unzippedCodeDir) { | ||
| rimraf.sync(this.unzippedCodeDir); | ||
| fs.rmSync(this.unzippedCodeDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant file and cleanup call.
if [ -f src/subCommands/local/impl/baseLocal.ts ]; then
echo "FOUND src/subCommands/local/impl/baseLocal.ts ($(wc -l < src/subCommands/local/impl/baseLocal.ts) lines)"
sed -n '440,490p' src/subCommands/local/_impl/baseLocal.ts 2>/dev/null || sed -n '440,490p' src/subCommands/local/impl/baseLocal.ts
else
echo "Missing expected file; searching for baseLocal.ts"
fd -a 'baseLocal\.ts$' .
fi
echo
echo "Search for temp cleanup patterns:"
rg -n "fs\.rmSync|unzippedCodeDir|after\\(|force: true|maxRetries|retryDelay" src/subCommands/local/impl/baseLocal.ts src || true
echo
echo "Node fs.rmSync option defaults/behavior probe:"
node - <<'JS'
const fs = require('fs');
const path = require('path');
const dir = fs.mkdtempSync('/tmp/local-cleanup-probe-');
fs.mkdirSync(path.join(dir, 'nested'), { recursive: true });
fs.writeFileSync(path.join(dir, 'nested', 'file'), 'x');
let thrown = false;
let err;
try {
fs.rmSync(dir, { recursive: true, force: true });
} catch (e) {
thrown = true;
err = e;
}
console.log(JSON.stringify({
thrownAtFirstCall: thrown,
errorCode: err && err.code,
remainingAfterFirstCall: fs.existsSync(dir),
currentFsRmSyncInvariants: {
nodeVersion: process.version,
rmSyncSupportsOptions: typeof fs.rmSync === 'function',
rmSyncSupportsRetryOptions: fs.rmSync.length === 2,
}
}, null, 2));
JSRepository: devsapp/fc3
Length of output: 9024
🌐 Web query:
Node.js fs.rmSync options maxRetries default EBUSY EPERM documentation
💡 Result:
In Node.js, the fs.rmSync method is used to remove files and directories [1]. Regarding the maxRetries option, the following points apply: Default Value: The default value for the maxRetries option is 0 [1][2]. Functionality: If an EBUSY, EMFILE, ENFILE, ENOTEMPTY, or EPERM error is encountered, Node.js will retry the operation if maxRetries is greater than 0 [1][2]. The operation is retried with a linear backoff wait of retryDelay milliseconds longer on each successive try [1][2]. Requirements: The maxRetries option is ignored unless the recursive option is also set to true [1][2]. Historical Note: In earlier versions of Node.js (specifically prior to v13.3.0), this option was known as maxBusyTries [1][3]. The retry logic is designed to handle transient errors during recursive directory removal [1][2]. If recursive mode is not enabled, the maxRetries setting has no effect [1][2].
Citations:
- 1: https://nodejs.org/api/fs.html
- 2: https://nodejs.org/api/fs.md
- 3: https://nodejs.org/docs/latest-v24.x/api/fs.html
Retry recursive temp-directory cleanup.
force only ignores missing paths, and fs.rmSync() retries EBUSY/EPERM/ENOTEMPTY paths only when maxRetries > 0. Set recursive, force, and maxRetries so after() does not fail the command on transient cleanup errors.
Proposed fix
- fs.rmSync(this.unzippedCodeDir, { recursive: true, force: true });
+ fs.rmSync(this.unzippedCodeDir, {
+ recursive: true,
+ force: true,
+ maxRetries: 3,
+ retryDelay: 100,
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fs.rmSync(this.unzippedCodeDir, { recursive: true, force: true }); | |
| fs.rmSync(this.unzippedCodeDir, { | |
| recursive: true, | |
| force: true, | |
| maxRetries: 3, | |
| retryDelay: 100, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/subCommands/local/impl/baseLocal.ts` at line 471, Update the cleanup call
in the after() method to configure fs.rmSync with recursive and force enabled
plus a positive maxRetries value, ensuring transient EBUSY, EPERM, and ENOTEMPTY
errors are retried without changing the existing target path.
Removing patch-package (which rewrote ali-oss's process.browser) made ts-jest type-check ali-oss's bundled .ts source and fail with TS2339, breaking npm test on all CI platforms. transformIgnorePatterns does not exclude node_modules, so ts-jest compiles the imported .ts. Scope ts-jest diagnostics to exclude node_modules, keeping type-checking for our own code while ignoring third-party .ts type errors. npm build and typecheck were unaffected (they resolve ali-oss's .js entry).
Resolved URLs pointed to registry.npmmirror.com, which GitHub runners time out reaching during npm install (ERR_SOCKET_TIMEOUT).
1 participant
Replace temp-dir, ip, rimraf, and shell helpers with Node APIs. Remove unused download and trigger-template code, declare runtime dependencies explicitly, and add Node 16 type checking.
Summary by CodeRabbit
Changes
Tests
Documentation