fix: fixes "url" argument must be of type string with nodejs 20 #378
Conversation
node:internal/errors:541
throw error;
^
TypeError [ERR_INVALID_ARG_TYPE]: The "url" argument must be of type string. Received undefined
at Url.parse (node:url:170:3)
at Object.urlParse [as parse] (node:url:141:13)
at new FtpServer (C:\work\xemu\xemu-bsp-list\ftp-srv\src\index.js:34:24)
at startFtpServer (C:\work\xemu\xemu-bsp-list\ftp-srv\bin\index.js:133:21)
at Object.<anonymous> (C:\work\xemu\xemu-bsp-list\ftp-srv\bin\index.js:11:1)
at Module._compile (node:internal/modules/cjs/loader:1369:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1427:10)
at Module.load (node:internal/modules/cjs/loader:1206:32)
at Module._load (node:internal/modules/cjs/loader:1022:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:135:12)
at node:internal/main/run_main_module:28:49 {
code: 'ERR_INVALID_ARG_TYPE'
}
Node.js v20.12.0
lygstate
force-pushed
the
main
branch
3 times, most recently
from
9a09651 to
89846a1
Compare
lygstate
changed the title
WalkthroughThis change updates type declarations (adding a "connect" event overload), refactors CommonJS exports to expose classes and individual errors, and sanitizes options on server construction by removing undefined values. No runtime logic changes beyond options filtering are introduced. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Server as FtpServer
participant Listener as "on('connect', ...)"
rect rgba(230,240,255,0.5)
Client->>Server: TCP connection established
note over Server: Accepts connection
Server-->>Listener: emit "connect" { connection, id, newConnectionCount }
note right of Listener: New overload in d.ts
end
opt Subsequent flow
Server-->>Client: Proceed with login/commands
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
![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)
src/index.js (1)
29-29: Filter null values in options
Use_.pickBy(options, v => v != null)to exclude bothundefinedandnull, preventingurl.parse(null)from throwing in Node.js 20.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ftp-srv.d.ts(2 hunks)ftp-srv.js(1 hunks)src/index.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/index.js (2)
src/commands/index.js (1)
_(1-1)src/connection.js (1)
_(1-1)
ftp-srv.d.ts (2)
src/connection.js (2)
FileSystem(7-7)EventEmitter(4-4)src/index.js (2)
tls(6-6)EventEmitter(7-7)
🔇 Additional comments (2)
ftp-srv.d.ts (1)
1-190: Formatting improvements enhance readability.The formatting changes throughout this file (consistent semicolons, multi-line signatures, aligned indentation) improve code readability without altering semantic types. These changes complement the API surface expansion in
ftp-srv.js.ftp-srv.js (1)
1-14: Reintroduce or document group error export and separate API refactor from bug fix
- The aggregate
errors/ftpErrorsexport was removed inftp-srv.js, breaking any consumers doingrequire('ftp-srv').errors; re-addmodule.exports.errors = errors(or aliasftpErrors) and note this in a migration guide.- The export changes are unrelated to the Node.js 20 URL fix—split them into a dedicated PR or update the title/description to cover both changes.
| on( | ||
| event: "connect", | ||
| listener: (data: { connection: FtpConnection; id: string; newConnectionCount: number }) => void | ||
| ): this; |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! The connect event overload aligns with runtime behavior.
The new on("connect", listener) overload correctly declares the event emitted at line 57 in src/index.js, providing proper TypeScript support for the connect event handler.
Verify that all event emissions in the runtime code have corresponding type declarations:
🏁 Script executed:
#!/bin/bash
# Find all emit calls in src/index.js and verify they have type declarations
rg -nP --type=js 'this\.emit\(' src/index.js
# Check for any other event emitters in the codebase
rg -nP --type=js 'this\.emit\(' src/Length of output: 1256
#!/bin/bash
evs=(disconnect connect server-error closing closed STOR RNTO RETR)
for ev in "${evs[@]}"; do
if rg -nP "on\(\sevent:\s"$ev"" ftp-srv.d.ts > /dev/null; then
echo "$ev: declared"
else
echo "$ev: missing"
fi
done
#!/bin/bash
Check for event overload declarations in ftp-srv.d.ts
evs=(disconnect connect server-error closing closed STOR RNTO RETR)
for ev in "${evs[@]}"; do
echo -n "$ev: "
rg -nP "on(\sevent:\s"$ev"" ftp-srv.d.ts || echo "missing"
done
🏁 Script executed:
#!/bin/bash
evs=(disconnect connect "server-error" closing closed STOR RNTO RETR)
for ev in "${evs[@]}"; do
if rg -nP "on\\(\\s*event:\\s*\"$ev\"" ftp-srv.d.ts; then
echo "$ev: declared"
else
echo "$ev: missing"
fi
doneLength of output: 133
🏁 Script executed:
#!/bin/bash
FILE=$(find . -type f -name "ftp-srv.d.ts" | head -1)
echo "Path: $FILE"
sed -n '150,200p' "$FILE"Length of output: 920
Add missing event overloads
ftp-srv.d.ts currently covers login, connect, disconnect, and client-error but omits the following runtime-emitted events in src/index.js:
"server-error"(payload{ error: Error })"closing"(no payload)"closed"(no payload)
Additionally, command-level emits in src/commands/registration ("STOR", "RNTO", "RETR") need corresponding on(event, listener) overloads on the FtpConnection type.
🤖 Prompt for AI Agents
In ftp-srv.d.ts around lines 169–172, add missing event overloads and
command-level emits: add on(event: "server-error", listener: (data: { error:
Error }) => void): this; add on(event: "closing", listener: () => void): this;
add on(event: "closed", listener: () => void): this; and on the FtpConnection
type add overloads for command-level emits on("STOR", listener: (data: any) =>
void): this; on("RNTO", listener: (data: any) => void): this; on("RETR",
listener: (data: any) => void): this; ensuring signatures match the runtime
payloads in src/index.js and src/commands/registration (replace any with more
specific types if present in those files).
Acceptance Checklist
Review Checklist
Fixes #377
Summary by CodeRabbit