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

feat: node tty, readline module #91

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

Conversation

Puellaquae
Copy link
Contributor

@Puellaquae Puellaquae commented Nov 22, 2022

Node's tty and readline module is implemented. getWindowSize and setRawMode is unsupported now.

As test, because test required assert module is in fs's pr, it will be added after fs's pr merged.

@Puellaquae Puellaquae changed the title feat: node tty module feat: node tty, readline module Nov 28, 2022
@juntao
Copy link
Member

juntao commented Apr 3, 2023

flows summarize

Copy link
Member

juntao commented Apr 3, 2023

Hello, I am a serverless review bot on flows.network. Here are my reviews of code commits in this PR.


The first patch adds new modules, including internal/tty.js and public/tty.js, with APIs that determine the color depth of the terminal and has no issues. The second patch adds several modules for handling user input through readline, but may have issues due to a new dependency on string_decoder and many new, untested functions. Testing thoroughly is recommended before merging.

Details

Commit 1

This patch adds an internal/tty.js module that provides a getColorDepth and a hasColors API along with a new public tty.js module which exports isatty, ReadStream, and WriteStream classes. The hasColors and getColorDepth APIs are fairly complex and appear to handle a wide range of environments by detecting various terminal emulators and reading environment variables to determine the number of colors that are supported. However, the associated internal and public modules contain no issues.

Commit 2

This patch adds several new modules related to handling user input through readline on Node.js. It includes implementations for moving the cursor, emitting keypress events, and clearing lines and screens. The patch also includes new validation and error handling code.

One potential problem is that the patch introduces a new dependency on the string_decoder module, but it does not check if the module is available before using it. This could result in issues on systems where the string_decoder module is not installed.

Another potential issue is that the patch introduces many new functions that may not have been properly tested, and they may have bugs or compatibility issues that could cause problems in production. It would be important to thoroughly test these new modules before merging the patch.

@juntao
Copy link
Member

juntao commented Apr 5, 2023

flows summarize

Copy link
Member

juntao commented Apr 5, 2023

Hello, I am a serverless review bot on flows.network. Here are my reviews of code commits in this PR.


Overall Summary:

This set of software source code patches introduces a new TTY module and a new readline module, along with multiple internal components, and modifies existing files to accommodate the changes. The most significant potential problems and errors include limited support for certain functionality and platform-specific code, a temporary solution for a missing class implementation, and a possible increase in bugs or unintended effects due to the large amount of new code.

Key findings:

  1. Limited functionality due to the lack of support for functions like tcgetattr, tcsetattr, ioctl, and tcgetwinsize. Additionally, there's a missing segment of code for Windows, which may lead to limited color support on the platform.

  2. The temporary solution of extending fs.ReadStream in place of the not-yet-implemented net.Socket may cause issues in the future once the proper implementation becomes available.

  3. The large amount of new code in the readline module increases the chances of potential bugs or unintended effects on existing functionality, and lacks test cases or updates to existing test cases to cover the new functionality.

To tackle these issues, it is advised to implement the missing functions, uncomment the platform-specific code for Windows, ensure full functionality and platform support, thoroughly test the new functionality, and review the patches for best practices and coding standards.

Details

Commit 1

Summary of key changes:

  • A new tty module has been implemented in the following files:
    • modules/internal/tty.js: Contains the primary implementation of utility functions like getColorDepth and hasColors.
    • modules/tty.js: Exports isatty, ReadStream, and WriteStream classes, which provide TTY-related utilities.
    • src/internal_module/tty.rs: Seems to be a stub file for Rust implementation.
    • Other files have minor modifications to include the new files.

Potential problems:

  1. Limited support for certain functionality:

    • The setRawMode method in ReadStream class does not support setting terminal modes because it requires functions like tcgetattr and tcsetattr or ioctl, which are not available.
    • The getWindowSize method in the WriteStream class does not provide actual window size values as it requires the tcgetwinsize function, which is not supported.
  2. Temporary solution for missing functionality:

    • The ReadStream class is currently extending fs.ReadStream temporarily, as the desired net.Socket is not yet implemented. This may cause issues in the future, especially when the proper implementation of net.Socket becomes available.
  3. Platform support:

    • The implementation is missing a segment of code for Windows, which is currently commented out. This may lead to limited support for colors on Windows and might need further investigation for proper Windows support.

To ensure full functionality, it is advised to implement the missing functions like tcgetattr, tcsetattr, ioctl, tcgetwinsize, and net.Socket, and uncomment the missing platform-specific code for Windows.

Commit 2

Summary of key changes:

  1. Implementing a new readline module with multiple internal components.
  2. Created several new files under modules/internal/readline such as callbacks.js, emitKeypressEvents.js, interface.js, promises.js, and utils.js.
  3. Modified existing files modules/internal/errors.js and modules/tty.js to accommodate changes related to the new readline module.
  4. Added new error class ERR_INVALID_CURSOR_POS in modules/internal/errors.js.
  5. Implemented functions for cursor movement, clearing lines, and emitting keypress events in callbacks.js, and emitKeypressEvents.js.

Potential problems:

  1. There is a large amount of new code being introduced, increasing the chances of potential bugs or unintended effects on existing functionality.
  2. The patch lacks test cases or updates to existing test cases to cover the new functionality being introduced.
  3. As multiple files have been modified, there might be integration or compatibility issues between the new readline module and existing modules.

To mitigate potential problems, it is recommended to:

  1. Thoroughly test the new functionality through unit and integration tests.
  2. Review the patch for adherence to best practices and coding standards.
  3. Ensure the new functionality does not adversely impact existing features or break backward compatibility.

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.

None yet

2 participants