microsoft / node-pty

Fork pseudoterminals in Node.JS
Other
1.43k stars 232 forks source link

Newer typescript incompatibility? #539

Closed SnarkBoojum closed 2 years ago

SnarkBoojum commented 2 years ago

There seem to be a number of typing problems with node-pty 0.10.0 and newer typescript (here: 4.6.3).

I can go further with the following patch:

--- node-node-pty.orig/src/terminal.ts
+++ node-node-pty/src/terminal.ts
@@ -137,7 +137,7 @@
   }

   /** See net.Socket.setEncoding */
-  public setEncoding(encoding: string | null): void {
+  public setEncoding(encoding: BufferEncoding | null): void {
     if ((this._socket as any)._decoder) {
       delete (this._socket as any)._decoder;
     }
@@ -187,7 +187,7 @@
   public abstract get slave(): Socket;

   protected _close(): void {
-    this._socket.writable = false;
+    //this._socket.writable = false;
     this._socket.readable = false;
     this.write = () => {};
     this.end = () => {};
--- node-node-pty.orig/src/unixTerminal.ts
+++ node-node-pty/src/unixTerminal.ts
@@ -73,7 +73,7 @@
     env.TERM = name;
     const parsedEnv = this._parseEnv(env);

-    const encoding = (opt.encoding === undefined ? 'utf8' : opt.encoding);
+    const encoding = (opt.encoding === undefined ? 'utf8' : opt.encoding) as BufferEncoding;

     const onexit = (code: number, signal: number): void => {
       // XXX Sometimes a data event is emitted after exit. Wait til socket is
@@ -186,7 +186,7 @@

     const cols = opt.cols || DEFAULT_COLS;
     const rows = opt.rows || DEFAULT_ROWS;
-    const encoding = (opt.encoding === undefined ? 'utf8' : opt.encoding);
+    const encoding = (opt.encoding === undefined ? 'utf8' : opt.encoding) as BufferEncoding;

     // open
     const term: IUnixOpenProcess = pty.open(cols, rows);
--- node-node-pty.orig/src/windowsPtyAgent.ts
+++ node-node-pty/src/windowsPtyAgent.ts
@@ -146,9 +146,9 @@

   public kill(): void {
     this._inSocket.readable = false;
-    this._inSocket.writable = false;
+    //this._inSocket.writable = false;
     this._outSocket.readable = false;
-    this._outSocket.writable = false;
+    //this._outSocket.writable = false;
     // Tell the agent to kill the pty, this releases handles to the process
     if (this._useConpty) {
       this._getConsoleProcessList().then(consoleProcessList => {
@@ -233,9 +233,9 @@

   private _cleanUpProcess(): void {
     this._inSocket.readable = false;
-    this._inSocket.writable = false;
+    //this._inSocket.writable = false;
     this._outSocket.readable = false;
-    this._outSocket.writable = false;
+    //this._outSocket.writable = false;
     this._outSocket.destroy();
   }
 }

where you'll notice most chunks are commenting out code: I don't know if the lines should just be dropped or if there's an api call to use - I just wanted to see if I could get further.

I'm still stuck with:

src/windowsPtyAgent.ts:188:25 - error TS2339: Property 'consoleProcessList' does not exist on type 'Serializable'.
  Property 'consoleProcessList' does not exist on type 'string'.

188         resolve(message.consoleProcessList);
                            ~~~~~~~~~~~~~~~~~~

I hope that helps.

Tyriar commented 2 years ago

Are these problems causing issues for you or were you just trying to update TS because it was older?

SnarkBoojum commented 2 years ago

Well, I'm packaging node-pty for Debian, so I need to be able to compile the code.

Tyriar commented 2 years ago

Oh I see, the package.json shows "typescript": "^3.8.3":

https://github.com/microsoft/node-pty/blob/1674722e1caf3ff4dd52438b70ed68d46af83a6d/package.json#L58

To get it working try running npm i -D typescript@3.8.3 to pin the right version.

SnarkBoojum commented 2 years ago

No, npm is a distribution, Debian is another: we don't use npm packages to build Debian ones.

And we start from sources, not pre-built binaries.

So I want to improve the code so it "works" with recent versions of tsc.

Tyriar commented 2 years ago

If the above works you can make a PR, the ^ means that package minor versions will be updated automatically and TS doesn't follow semantic versioning which breaks this model.

SnarkBoojum commented 2 years ago

The above:

  1. has lines where I just commented writing to the writable attribute of sockets (it's now read-only) and I'm not sure if those should just be dropped or if there's something else to do -- that part is not clean ;
  2. has good chunks (all places where the word BufferEncoding appears) ;
  3. there's still the problem with line 188 of src/windowsPtyAgent.ts where I really don't know what to do.
Tyriar commented 2 years ago

Updating/validating typescript will take more effort than I'm willing to invest right now as I have too much to do. Looking into this just now there is no problem. We use yarn.lock to pin dependencies and that ensures that npm/yarn will only install typescript 3.8.3:

https://github.com/microsoft/node-pty/blob/e6255446e5de909f3d57455dae57b07c54cd7fdd/yarn.lock#L1393-L1397

You seem to be assuming you can take a library and build it with the latest version of typescript, that's not how it works and you're bound to run into problems building code that has not been validating with the newer compiler. It's similar to taking a Python 2 program and assuming it will work in Python 3, since tsc contains breaking changes.

I appreciate you want to improve the code but it's not necessary, things work fine with the older version of tsc and it still compiles valid JS.