Skip to content

Add process.stdin.setRawMode - #2046

Closed
ThatOneBro wants to merge 22 commits into
mainfrom
derrick/feat/add-tty-raw-mode
Closed

ThatOneBro wants to merge 22 commits into
mainfrom
derrick/feat/add-tty-raw-mode

Conversation

@ThatOneBro

@ThatOneBro ThatOneBro commented Feb 11, 2023 •

Copy link
Copy Markdown
Contributor

Still need to finish wiring things and make sure the mode is set properly for a few different cases (namely, async iteration of console, ie:

for await (const line of console)...

and also for node:readline interfaces)
but this is nearly ready.

Closes #2025 and closes #2239

Comment thread src/bun.js/builtins/js/ProcessObjectInternals.js Outdated
Comment thread src/bun_js.zig Outdated
@ThatOneBro
ThatOneBro force-pushed the derrick/feat/add-tty-raw-mode branch 2 times, most recently from 77d202b to 45abcb8 Compare February 18, 2023 22:44
@ThatOneBro

Copy link
Copy Markdown
Contributor Author

Update on this PR: Got process.stdin.isRaw and process.stdin.setRawMode working now, though I do need to add some tests to verify that it's working in all cases (such as when we try to do read from async iter on console). I don't think this will currently work, as I probably still have to set raw mode for the duration of that iter maybe... We'll see.

I'll come back to this PR to add tests after I finish the undici polyfills, which should be tonight.

@ThatOneBro
ThatOneBro force-pushed the derrick/feat/add-tty-raw-mode branch from 4d22237 to 8f66641 Compare March 15, 2023 18:20
@ThatOneBro
ThatOneBro marked this pull request as ready for review March 18, 2023 06:04
@ThatOneBro ThatOneBro changed the title [WIP] Add process.stdin.setRawMode Add process.stdin.setRawMode Mar 18, 2023
@ThatOneBro
ThatOneBro force-pushed the derrick/feat/add-tty-raw-mode branch from 4e7a511 to 2859820 Compare March 18, 2023 07:18
@github-actions

github-actions Bot commented Mar 18, 2023 •

Copy link
Copy Markdown
Contributor

✅ prettier errors have been resolved. Thank you.

#36a44e54521594043f1b3c0274f1b948ee66fc5f

@github-actions

github-actions Bot commented Mar 18, 2023 •

Copy link
Copy Markdown
Contributor

❌ @ThatOneBro 29 files with test failures on bun-darwin-x64-baseline:

  • test/bundler/transpiler.test.js
  • test/js/bun/console/console-iterator.test.js
  • test/js/bun/http/serve.test.ts
  • test/js/bun/net/socket.test.ts
  • test/js/bun/resolve/png/test-png-import.test.js
  • test/js/bun/resolve/resolve.test.js
  • test/js/bun/spawn/spawn.test.ts
  • test/js/bun/sqlite/sqlite.test.js
  • test/js/bun/util/escapeHTML.test.js
  • test/js/bun/util/sleepSync.test.ts
  • test/js/deno/crypto/webcrypto.test.ts
  • test/js/deno/encoding/encoding.test.ts
  • test/js/deno/fetch/headers.test.ts
  • test/js/deno/fetch/request.test.ts
  • test/js/deno/fetch/response.test.ts
  • test/js/deno/url/url.test.ts
  • test/js/deno/v8/error.test.ts
  • test/js/node/child_process/child_process-node.test.js
  • test/js/node/disabled-module.test.js
  • test/js/node/fs/fs.test.ts
  • test/js/node/process/process-stdio.test.ts
  • test/js/node/process/process.test.js
  • test/js/node/util/util-promisify.test.js
  • test/js/node/util/util.test.js
  • test/js/third_party/napi_create_external/napi-create-external.test.ts
  • test/js/web/abort/abort.test.ts
  • test/js/web/fetch/body.test.ts
  • test/regression/issue/00631.test.ts
  • test/regression/issue/02368.test.ts

View test output

#36a44e54521594043f1b3c0274f1b948ee66fc5f

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

needs a small rebase but looks close to mergeable

nice

if (result < 0) {
JSC::throwException(
globalObject, throwScope,
JSC::createError(globalObject, "Failed to set tty mode. Error code: "_s + WTFMove(std::to_string(result).c_str())));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use makeString instead of + operator

if (result < 0) {
JSC::throwException(
globalObject, throwScope,
JSC::createError(globalObject, "Failed to set tty mode. Error code: "_s + WTFMove(std::to_string(result).c_str())));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use makeString instead of + operator


namespace Zig {

static bool tty__orig_set = false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break when we have multiple instances of Bun's runtime

Sort of need this to be part of JSTTYHelper as an object with the relevant fields

_ = std.c.tcgetattr(fd, &termios);
bun.C.cfmakeraw(&termios);
// var termios = std.mem.zeroes(std.os.termios);
// _ = std.c.tcgetattr(fd, &termios);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to do this for the non-readline case right?

What if you do Bun.stdin.getReader()?

@Electroid

Copy link
Copy Markdown
Contributor

setRawMode was implemented in Bun v0.8.0.

@Electroid Electroid closed this Oct 17, 2023
@Electroid
Electroid deleted the derrick/feat/add-tty-raw-mode branch October 17, 2023 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add pty utils for testing Implement process.stdin.setRawMode (and ReadlineInterface.setRawMode)

3 participants