Conversation
|
Seemingly this does not work on Windows, which hangs. I didn't exactly test there, so, fair. |
| stdout._handle.setBlocking?.(true); | ||
| stdin._handle.setBlocking?.(true); |
There was a problem hiding this comment.
This may look sketchy, but this is what tsc has done all along, in fact
24c623b to
826838f
Compare
826838f to
204ce2a
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes the Rust/N-API-based @typescript/libsyncrpc dependency for the synchronous JS API by implementing a pure-JS synchronous RPC channel (using fs.readSync/fs.writeSync on raw fds) and adding a pipe/socket transport option on the Go API server (used on Windows).
Changes:
- Replace
@typescript/libsyncrpcwith a pure JSSyncRpcChannelimplementation and wire it into the API client. - Add
--pipesupport to thetsgo --apiserver so clients can connect over a named pipe (Windows) / Unix domain socket instead of stdio. - Remove Rust requirements from docs, devcontainer, and CI workflows; adjust benchmark iteration settings to reduce runtime.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
package-lock.json |
Removes @typescript/libsyncrpc from the lockfile. |
internal/api/server.go |
Adds PipePath option and chooses pipe vs stdio transport at runtime. |
cmd/tsgo/api.go |
Adds --pipe flag and configures server options accordingly. |
_packages/api/src/syncChannel.ts |
New pure-JS synchronous RPC implementation replacing the native addon. |
_packages/api/src/client.ts |
Switches sync client to use the new local SyncRpcChannel. |
_packages/api/package.json |
Drops dependency on @typescript/libsyncrpc. |
_packages/api/test/api.sync.bench.ts |
Reduces benchmark iterations/warmup to shorten runtime. |
_packages/api/test/api.async.bench.ts |
Same benchmark iteration/warmup reduction for async benches. |
CONTRIBUTING.md |
Removes Rust requirement from contributor setup docs. |
.github/workflows/create-cache.yml |
Removes Rust toolchain setup step. |
.github/workflows/copilot-setup-steps.yml |
Removes Rust toolchain setup step. |
.github/workflows/ci.yml |
Removes Rust toolchain setup steps from CI jobs. |
.devcontainer/devcontainer.json |
Removes Rust devcontainer feature. |
| } else { | ||
| t := NewStdioTransport(s.options.In, s.options.Out) | ||
| defer t.Close() | ||
| transport = t |
There was a problem hiding this comment.
When PipePath is empty, this branch constructs a StdioTransport even if options.In/options.Out are nil. That will lead to a nil Reader/Writer and a later panic or confusing I/O error. Consider validating at startup that either PipePath is set OR both In and Out are non-nil, and return a clear error (or panic) if the configuration is invalid.
| if s.options.PipePath != "" { | ||
| t, err := NewPipeTransport(s.options.PipePath) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create pipe transport: %w", err) |
There was a problem hiding this comment.
Security/data-loss risk: on Unix, NewPipeTransport ultimately calls newPipeListener which unconditionally os.Remove()s the provided path before listening. Now that PipePath is user-controllable (via --pipe), passing an arbitrary path could delete a regular file. Consider only removing an existing file if it is a Unix socket (or refusing to overwrite non-socket paths), and ideally clean up the socket file on shutdown as well.
There was a problem hiding this comment.
this is not a security boundary; if you can run tsgo with weird args, you can run rm with bad args
| const pipePath = `\\\\.\\pipe\\tsgo-sync-${process.pid}-${Date.now()}`; | ||
| this.child = spawn(exe, [...args, "--pipe", pipePath], { | ||
| stdio: ["ignore", "ignore", "inherit"], | ||
| }); |
There was a problem hiding this comment.
spawn() can emit an 'error' event (e.g. ENOENT when the executable path is wrong). With no listener attached, Node will treat it as an unhandled 'error' event and crash the process. Consider adding a one-time 'error' handler that captures the spawn failure and surfaces it as a regular thrown error to the caller (and/or closes the channel).
| catch { | ||
| if (this.child.exitCode !== null) { | ||
| throw new Error( | ||
| `Child process exited with code ${this.child.exitCode} before pipe was ready`, | ||
| ); |
There was a problem hiding this comment.
The Windows pipe connect loop retries openSync for up to ~5s, but the bare catch {} swallows all errors (including permission/path-format errors) and will eventually report a generic timeout. Capturing the exception and only retrying on the expected transient errors (and surfacing others immediately) would make failures much easier to diagnose.
This is something I've wanted to try for a while, but was always unsuccessful at. This time, it finally works.
For our sync API, we use a
napi-rsmodule to do what Node largely can't; synchronous communication to a child process. This is good and all, but complicates things because in addition to publishing a multi-platform package with Go, we also have to do that with Rust. That and, we all need to have rust installed, the package changes integrity every install, etc.I've long wanted to try using Node's
fs.writeSync/fs.readSyncon raw file descriptors to do the same thing, in pure JS. Most of my attempts failed, or were seemingly slow.However, this attempt did not seem to fail (at least on my machine). A summary copied from a temporarily-committed markdown file:
The pure JS version is effectively the same as going through the native code, except for startup time.
The async code looks mega fast due the fact that it can async start a process and then you can get a response later, but it's not like it's doing less work, so I don't know if that is actually a downside. But clearly, Node has a huge overhead for spawning a process that a NAPI extension does not need.