-
Notifications
You must be signed in to change notification settings - Fork 235
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
Port to NAPI #644
Port to NAPI #644
Conversation
The "5th pty bug" in #432 fixed also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good to me, request for a review. @Tyriar @DavidRusso
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! |
Awesome work! Looking forward to this to land to support Node v21.x |
The macOS arm64 test successful here. |
@deepak1556 @rzhao271 a review for this would be appreciated when you have the time |
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});
// 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 |
@rajialtooro See oven-sh/bun#7685 . IMO this is a bunjs's bug. |
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
And, merry christmas! |
Hi, is there any progress on reviews? 😆 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking the time to work on this refactor!
Sorry for the delayed review, changes LGTM mostly. Left a few style comments, should be good to merge after that.
https://github.com/kkocdko/utils4linux/actions/runs/7645893929/job/20835695678 All things done, perhaps. |
Looks like there are build failures on windows and test failures on macOS |
@deepak1556 Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
onexit
support. Thanks to DavidRusso.onexit
, fix the "5th pty bug" in Port project to N-API #432 .__APPLE__
section inpty_waitpid
.Need no more to say, mainly about compatibility.
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.