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

modules: add tty #675

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

modules: add tty #675

wants to merge 2 commits into from

Conversation

ahaoboy
Copy link
Contributor

@ahaoboy ahaoboy commented Nov 13, 2024

Description of changes

Add tty module and use libc to implement tty.isatty function

Checklist

  • Created unit tests in tests/unit and/or in Rust for my feature if needed
  • Ran make fix to format JS and apply Clippy auto fixes
  • Made sure my code didn't add any additional warnings: make check
  • Added relevant type info in types/ directory
  • Updated documentation if needed (API.md/README.md/Other)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@richarddavison
Copy link
Contributor

@ahaoboy tests are failing because you need to mark it as external in esbuild config inside build.mjs

Comment on lines 3 to 17
describe("tty", () => {
it("isatty", () => {
// FIXME: in the test environment, isatty is always false
expect([
isatty(1),
isatty(2),
isatty(3),
isatty(4),
]).toEqual([
false,
false,
false,
false,
]);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I get the same results if I run it on the laptop at hand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The results of running the command line are correct. I suspect that llrt test uses redirection or other unknown behaviors, which causes all false to be returned in the test.

import { isatty } from 'tty'

console.log(
  isatty(1),
  isatty(2),
  isatty(3),
  isatty(4),
)
llrt ./a.js

true true false false

Copy link
Contributor

Choose a reason for hiding this comment

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

I figured that would probably be the case. This is a little troubling because I always run the test suite on the laptop at hand to make sure it is error free before shipping...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now only the return value type is checked

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.

3 participants