Skip to content

chore(cli): Replace node-pty with simple interactive process#55

Draft
jsteinich wants to merge 4 commits intoopen-constructs:mainfrom
jsteinich:replace_node-pty
Draft

chore(cli): Replace node-pty with simple interactive process#55
jsteinich wants to merge 4 commits intoopen-constructs:mainfrom
jsteinich:replace_node-pty

Conversation

@jsteinich
Copy link
Collaborator

@jsteinich jsteinich commented Mar 14, 2026

Related issue

Fixes #4

Description

  • Built a simple interactive process wrapper that makes use of cross-spawn with the inherit option for stdin
  • Replaced exiting pty-process wrapper with new interactive-process wrapper
  • Replaced direct node-pty usage with cross-spawn usage
  • Removed references to node-pty

Checklist

  • I have updated the PR title to match CDKTN's style guide
  • I have run the linter on my code locally
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if applicable
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works if applicable
  • New and existing unit tests pass locally with my changes

import { EOL } from "os";
import { spawnPty } from "./pty-process";

interface PtySpawnConfig {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was effectively a duplicate interface, so just importing it's replacement.

timeout?: number,
) => Promise<string>;
} => {
let disposables: IDisposable[] = [];
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this is necessary any more.

const child = crossSpawn("cdktn", ["watch", "--auto-approve"], {
env: this.env,
cols: 80,
rows: 60,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't supported any more as there isn't a pty

@commiterate
Copy link

commiterate commented Mar 14, 2026

node:child_process v. cross-spawn (since it wasn't clear to me what cross-spawn actually does from its README): sindresorhus/execa#578 (comment)

Since CDKTF was already using it (and thus CDKTN as well), this is fine in the short to medium term.

Long term I'm less sure because cross-spawn seems unmaintained and there's some concerning issues like moxystudio/node-cross-spawn#175.

I'm not completely sure which parts of cross-spawn are important to CDKTN. It looks like:

  • cdktn invocations already split arguments (e.g. spawn("cdktn", ["watch", "--auto-approve"]).
  • Using spawn(..., { shell: process.platform === 'win32' }) seems fine for this use case specifically (automatically toggles quoting/escaping and PATHEXT is handled by the Windows shell).

@jsteinich
Copy link
Collaborator Author

node:child_process v. cross-spawn (since it wasn't clear to me what cross-spawn actually does from its README): sindresorhus/execa#578 (comment)

Since CDKTF was already using it (and thus CDKTN as well), this is fine in the short to medium term.

Long term I'm less sure because cross-spawn seems unmaintained and there's some concerning issues like moxystudio/node-cross-spawn#175.

I'm not completely sure which parts of cross-spawn are important to CDKTN. It looks like:

  • cdktn invocations already split arguments (e.g. spawn("cdktn", ["watch", "--auto-approve"]).
  • Using spawn(..., { shell: process.platform === 'win32' }) seems fine for this use case specifically (automatically toggles quoting/escaping and PATHEXT is handled by the Windows shell).

I used it here primarily because it was used elsewhere in the project, but I do suspect that some of the cross platform issues do come up.

At least of the portion of the Windows tests are currently disabled in CI. Would be nice to have them running before attempting to move away from cross-spawn, though I could run them locally on WIndows to check.

@commiterate
Copy link

Sounds good. Dropping cross-spawn can be tracked separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cdktn-cli: Invoke Terraform CLI with node:child_process instead of node-pty. (Bun + Deno support)

2 participants