Make the Pty constructor take an object (#10)

The Pty constructor is going to grow soon, so might as well make this refactor now. This change makes some of the parameters to Pty optional for ergonomics. --------- Co-authored-by: Szymon Kaliski <[email protected]>
replit · May 6, 2024 · 443152a · 443152a
1 parent 06c940a
commit 443152a
Show file tree

Hide file tree

Showing 3 changed files with 86 additions and 71 deletions.
diff --git a/index.d.ts b/index.d.ts
@@ -3,6 +3,15 @@
 
 /* auto-generated by NAPI-RS */
 
+/** The options that can be passed to the constructor of Pty. */
+export interface PtyOptions {
+  command: string
+  args?: Array<string>
+  envs?: Record<string, string>
+  dir?: string
+  size?: Size
+  onExit: (err: null | Error, exitCode: number) => void
+}
 /** A size struct to pass to resize. */
 export interface Size {
   cols: number
@@ -57,7 +66,7 @@ export interface Size {
 export class Pty {
   /** The pid of the forked process. */
   pid: number
-  constructor(command: string, args: Array<string>, envs: Record<string, string>, dir: string, size: Size, onExit: (err: null | Error, exitCode: number) => void)
+  constructor(opts: PtyOptions)
   /** Resize the terminal. */
   resize(size: Size): void
   /**

diff --git a/index.test.ts b/index.test.ts
@@ -64,21 +64,18 @@ describe('PTY', () => {
     const message = 'hello from a pty';
     let buffer = '';
 
-    const pty = new Pty(
-      '/bin/echo',
-      [message],
-      {},
-      CWD,
-      { rows: 24, cols: 80 },
-      (err, exitCode) => {
+    const pty = new Pty({
+      command: '/bin/echo',
+      args: [message],
+      onExit: (err, exitCode) => {
         expect(err).toBeNull();
         expect(exitCode).toBe(0);
         expect(buffer).toBe(message + '\r\n');
         pty.close();
 
         done();
       },
-    );
+    });
 
     const readStreamFd = pty.fd();
     const readStream = fs.createReadStream('', { fd: readStreamFd });
@@ -96,20 +93,17 @@ describe('PTY', () => {
   });
 
   test('captures an exit code', (done) => {
-    let pty = new Pty(
-      '/bin/sh',
-      ['-c', 'exit 17'],
-      {},
-      CWD,
-      { rows: 24, cols: 80 },
-      (err, exitCode) => {
+    const pty = new Pty({
+      command: '/bin/sh',
+      args: ['-c', 'exit 17'],
+      onExit: (err, exitCode) => {
         expect(err).toBeNull();
         expect(exitCode).toBe(17);
         pty.close();
 
         done();
       },
-    );
+    });
   });
 
   test('can be written to', (done) => {
@@ -123,13 +117,16 @@ describe('PTY', () => {
       32, 99, 97, 116, 13, 10,
     ]);
 
-    const pty = new Pty('/bin/cat', [], {}, CWD, { rows: 24, cols: 80 }, () => {
-      // We have local echo enabled, so we'll read the message twice.
-      assert(buffer);
-      expect(macOSLinuxCatBufferCompare(buffer, result)).toBe(true);
-      pty.close();
+    const pty = new Pty({
+      command: '/bin/cat',
+      onExit: () => {
+        // We have local echo enabled, so we'll read the message twice.
+        assert(buffer);
+        expect(macOSLinuxCatBufferCompare(buffer, result)).toBe(true);
+        pty.close();
 
-      done();
+        done();
+      },
     });
 
     const readStream = fs.createReadStream('', { fd: pty.fd() });
@@ -157,10 +154,14 @@ describe('PTY', () => {
   });
 
   test('can be resized', (done) => {
-    const pty = new Pty('/bin/sh', [], {}, CWD, { rows: 24, cols: 80 }, () => {
-      pty.close();
+    const pty = new Pty({
+      command: '/bin/sh',
+      size: { rows: 24, cols: 80 },
+      onExit: () => {
+        pty.close();
 
-      done();
+        done();
+      },
     });
 
     const readStream = fs.createReadStream('', { fd: pty.fd() });
@@ -202,21 +203,17 @@ describe('PTY', () => {
   test('respects working directory', (done) => {
     let buffer = '';
 
-    const pty = new Pty(
-      '/bin/pwd',
-      [],
-      {},
-      CWD,
-      { rows: 24, cols: 80 },
-      (err, exitCode) => {
+    const pty = new Pty({
+      command: '/bin/pwd',
+      onExit: (err, exitCode) => {
         expect(err).toBeNull();
         expect(exitCode).toBe(0);
         expect(buffer).toBe(`${CWD}\r\n`);
         pty.close();
 
         done();
       },
-    );
+    });
 
     const readStream = fs.createReadStream('', { fd: pty.fd() });
 
@@ -235,15 +232,13 @@ describe('PTY', () => {
     const message = 'hello from env';
     let buffer: Buffer | undefined;
 
-    const pty = new Pty(
-      '/bin/sh',
-      ['-c', 'echo $ENV_VARIABLE && exit'],
-      {
+    const pty = new Pty({
+      command: '/bin/sh',
+      args: ['-c', 'echo $ENV_VARIABLE && exit'],
+      envs: {
         ENV_VARIABLE: message,
       },
-      CWD,
-      { rows: 24, cols: 80 },
-      (err, exitCode) => {
+      onExit: (err, exitCode) => {
         expect(err).toBeNull();
         expect(exitCode).toBe(0);
         assert(buffer);
@@ -252,7 +247,7 @@ describe('PTY', () => {
 
         done();
       },
-    );
+    });
 
     const readStream = fs.createReadStream('', { fd: pty.fd() });
 
@@ -277,14 +272,17 @@ describe('PTY', () => {
       111, 32, 98, 117, 110, 13, 10,
     ]);
 
-    const pty = new Pty('/bin/cat', [], {}, CWD, { rows: 24, cols: 80 }, () => {
-      // We have local echo enabled, so we'll read the message twice. Furthermore, the newline
-      // is converted to `\r\n` in this method.
-      assert(buffer !== undefined);
-      expect(macOSLinuxCatBufferCompare(buffer, result)).toBe(true);
-      pty.close();
+    const pty = new Pty({
+      command: '/bin/cat',
+      onExit: () => {
+        // We have local echo enabled, so we'll read the message twice. Furthermore, the newline
+        // is converted to `\r\n` in this method.
+        assert(buffer !== undefined);
+        expect(macOSLinuxCatBufferCompare(buffer, result)).toBe(true);
+        pty.close();
 
-      done();
+        done();
+      },
     });
 
     const file = Bun.file(pty.fd());
@@ -305,14 +303,10 @@ describe('PTY', () => {
 
   test("doesn't break when executing non-existing binary", (done) => {
     try {
-      new Pty(
-        '/bin/this-does-not-exist',
-        [],
-        {},
-        CWD,
-        { rows: 24, cols: 80 },
-        () => {},
-      );
+      new Pty({
+        command: '/bin/this-does-not-exist',
+        onExit: () => {},
+      });
     } catch (e) {
       expect(e.message).toContain('No such file or directory');
 

diff --git a/src/lib.rs b/src/lib.rs
@@ -71,6 +71,18 @@ struct Pty {
   pub pid: u32,
 }
 
+/// The options that can be passed to the constructor of Pty.
+#[napi(object)]
+struct PtyOptions {
+  pub command: String,
+  pub args: Option<Vec<String>>,
+  pub envs: Option<HashMap<String, String>>,
+  pub dir: Option<String>,
+  pub size: Option<Size>,
+  #[napi(ts_type = "(err: null | Error, exitCode: number) => void")]
+  pub on_exit: JsFunction,
+}
+
 /// A size struct to pass to resize.
 #[napi(object)]
 struct Size {
@@ -121,25 +133,20 @@ fn set_nonblocking(fd: c_int) -> Result<(), napi::Error> {
 impl Pty {
   #[napi(constructor)]
   #[allow(dead_code)]
-  pub fn new(
-    env: Env,
-    command: String,
-    args: Vec<String>,
-    envs: HashMap<String, String>,
-    dir: String,
-    size: Size,
-    #[napi(ts_arg_type = "(err: null | Error, exitCode: number) => void")] on_exit: JsFunction,
-  ) -> Result<Self, napi::Error> {
+  pub fn new(env: Env, opts: PtyOptions) -> Result<Self, napi::Error> {
     let should_dup_fds = env.get_node_version()?.release == "node";
+    let size = opts.size.unwrap_or(Size { cols: 80, rows: 24 });
     let window_size = Winsize {
       ws_col: size.cols,
       ws_row: size.rows,
       ws_xpixel: 0,
       ws_ypixel: 0,
     };
 
-    let mut cmd = Command::new(command);
-    cmd.args(args);
+    let mut cmd = Command::new(opts.command);
+    if let Some(args) = opts.args {
+      cmd.args(args);
+    }
 
     let rustix_openpty::Pty {
       controller: controller_fd,
@@ -161,8 +168,12 @@ impl Pty {
     cmd.stderr(Stdio::from(user_fd.try_clone()?));
     cmd.stdout(Stdio::from(user_fd.try_clone()?));
 
-    cmd.envs(envs);
-    cmd.current_dir(dir);
+    if let Some(envs) = opts.envs {
+      cmd.envs(envs);
+    }
+    if let Some(dir) = opts.dir {
+      cmd.current_dir(dir);
+    }
 
     unsafe {
       let raw_user_fd = user_fd.as_raw_fd();
@@ -212,7 +223,8 @@ impl Pty {
     //   they are ready to be `wait`'ed. This has the inconvenience that it consumes one FD per child.
     //
     // For discussion check out: https://github.com/replit/ruspty/pull/1#discussion_r1463672548
-    let ts_on_exit: ThreadsafeFunction<i32, ErrorStrategy::CalleeHandled> = on_exit
+    let ts_on_exit: ThreadsafeFunction<i32, ErrorStrategy::CalleeHandled> = opts
+      .on_exit
       .create_threadsafe_function(0, |ctx| ctx.env.create_int32(ctx.value).map(|v| vec![v]))?;
     thread::spawn(move || match child.wait() {
       Ok(status) => {