Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Newer typescript incompatibility? #539

Closed
SnarkBoojum opened this issue May 3, 2022 · 7 comments
Closed

Newer typescript incompatibility? #539

SnarkBoojum opened this issue May 3, 2022 · 7 comments
Labels
as-designed bug Issue identified by VS Code Team member as probable bug

Comments

@SnarkBoojum
Copy link

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
Copy link
Member

Tyriar commented May 3, 2022

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

@SnarkBoojum
Copy link
Author

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

@Tyriar
Copy link
Member

Tyriar commented May 3, 2022

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

"typescript": "^3.8.3"

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

@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug and removed need more info labels May 3, 2022
@SnarkBoojum
Copy link
Author

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
Copy link
Member

Tyriar commented May 3, 2022

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
Copy link
Author

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
Copy link
Member

Tyriar commented May 3, 2022

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:

node-pty/yarn.lock

Lines 1393 to 1397 in e625544

typescript@^3.8.3:
version "3.8.3"
resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.8.3.tgz#409eb8544ea0335711205869ec458ab109ee1061"
integrity sha512-MYlEfn5VrLNsgudQTVJeNaQFUAI7DkhnOjdpAp4T+ku1TfQClewlbSuTVHiA+8skNBgaf02TL/kLOvig4y3G8w==

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as-designed bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

No branches or pull requests

2 participants