Port to NAPI - Githubissues

kkocdko commented 7 months ago

[x] Port current code to NAPI, but ignore onexit support. Thanks to DavidRusso.
[x] Implement onexit, fix the "5th pty bug" in #432 .
[x] Port __APPLE__ section in pty_waitpid.
[x] Port windows part.
[x] Test on CI in my temp repo.

Need no more to say, mainly about compatibility.

NodeJS version (#557): Before this, only the version that vscode used. After this, from 16.x to future (21.x currently).
V8 version: Electron combines latest V8 with LTS NodeJS.
Pointer compression: Electron and some user (me) want pointer compression, use NAN needs recompile.
Bun (#632): NAPI was supported in Bun runtime, but NAN was not. However the NAPI in Bun seems still unstable.

About "5th pty bug" in #432

The Napi::AsyncWorker spawn a libuv worker thread which is limited by UV_THREADPOOL_SIZE (default = 4). It's more suitable for IO operations that need this limitation.

I want to taste now!

This PR only modify the native code, so just download from CI (see "Artifacts") then replace the origin files in ./build/Release/. Looking for macOS user because I have no macOS device.

panowl commented 7 months ago

GREAT JOB! I am writing a custom remote serial com program with electron, previously I could only download an extra node 18 for node-pty, after replacing the file with artifact in ci it now runs directly on electron!

exsilium commented 7 months ago

Awesome work! Looking forward to this to land to support Node v21.x

kkocdko commented 7 months ago

The macOS arm64 test successful here.

Tyriar commented 7 months ago

@deepak1556 @rzhao271 a review for this would be appreciated when you have the time

rajialtooro commented 6 months ago

Wonderful work on this PR @kkocdko

I'm facing this issue with trying to spawn a pty process like this using bunjs, it works normally with node

// code
import { spawn } from 'node-pty';

const term - spawn("bash", [], {name: "xterm", cols: 80, rows: 24});

# error
Error: Usage: pty.fork(file, args, env, cwd, cols, rows, uid, gid, utf8, helperPath, onexit)
    at <anonymous> (native)
    at new UnixTerminal (/application/node_modules/node-pty/lib/unixTerminal.js:104:14)

// package.json
"scripts": {
    "postinstall": "rm -rf ./node_modules/node-pty/build/Release/pty.node && mkdir -p ./node_modules/node-pty/build/Release  && cp ./pty.node ./node_modules/node-pty/build/Release/pty.node"
},
"dependencies": {
    "node-pty": "https://github.com/kkocdko/node-pty.git",
}

I also tried it with the normal node-pty package but got the same error. do you have any ideas if this is a misconfiguration on my end or a bug ?

kkocdko commented 6 months ago

@rajialtooro See https://github.com/oven-sh/bun/issues/7685 . IMO this is a bunjs's bug.

kkocdko commented 6 months ago

I think we should skip bun support, cross version node/electron support is good enough (for most use case).

Bun's NAPI is still weak and unreliable (maybe), after that bun #7685 bug, now I hit another:

napi: napi_define_properties
  Error::Error

uh-oh: napi: napi_define_properties
  Error::Error
bun will crash now 😭😭😭

----- bun meta -----
Bun v1.0.20 (09d51486) Linux x64 #18~22.04.1-Ubuntu SMP Tue Nov 21 19:25:02 UTC 2023
TestCommand: 
Elapsed: 130ms | User: 54ms | Sys: 39ms
RSS: 44.07MB | Peak: 57.67MB | Commit: 44.07MB | Faults: 0
----- bun meta -----

0   0x559017618a7b
1   ???
2   ???
3   ???
4   ???
5   ???
6   ???
7   ??? napi_fatal_error
8   ???
9   ??? Napi::Error::New(napi_env__*)
...

And, merry christmas!

kkocdko commented 5 months ago

Hi, is there any progress on reviews? 😆

kkocdko commented 5 months ago

https://github.com/kkocdko/utils4linux/actions/runs/7645893929/job/20835695678

All things done, perhaps.

deepak1556 commented 5 months ago

Looks like there are build failures on windows and test failures on macOS

kkocdko commented 5 months ago

@deepak1556 Fixed.

microsoft / node-pty

Port to NAPI #644

About "5th pty bug" in #432

I want to taste now!