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.

Sign up for GitHub

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

Jump to bottom

Implement node:tty #20855

Closed
littledivy opened this issue Oct 9, 2023 · 3 comments · Fixed by #20892
Closed

Implement node:tty #20855

littledivy opened this issue Oct 9, 2023 · 3 comments · Fixed by #20892
Assignees
@littledivy
Labels
node compat

Comments

@littledivy
Copy link
Member

littledivy commented Oct 9, 2023 •
edited
Loading

All of these reports below are blocked on node:tty support.
@littledivy littledivy self-assigned this Oct 11, 2023
@littledivy littledivy mentioned this issue Oct 12, 2023
Merged
littledivy added a commit that referenced this issue Oct 30, 2023
@littledivy
fix: implement node:tty (#20892)
0920410 
Fixes #21012
Closes #20855
Fixes #20890
Fixes #20611
Fixes #20336
Fixes `create-svelte` from #17248

Fixes more reports here:
- #6529 (comment)
- #6529 (comment)
- #6529 (comment)
bartlomieju pushed a commit to bartlomieju/deno that referenced this issue Oct 30, 2023
@littledivy @bartlomieju
fix: implement node:tty (denoland#20892)
f62a652 
Fixes denoland#21012
Closes denoland#20855
Fixes denoland#20890
Fixes denoland#20611
Fixes denoland#20336
Fixes `create-svelte` from denoland#17248

Fixes more reports here:
- denoland#6529 (comment)
- denoland#6529 (comment)
- denoland#6529 (comment)
@ulken
Copy link

ulken commented Nov 15, 2023 •
edited
Loading

@littledivy Not sure where to post this, but node:tty doesn't seem completely implemented. I'm trying to use term.ink and it crashes with:

  ERROR  unreachable

 ext:deno_node/_util/asserts.ts:16:9

 - unreachable (ext:deno_node/_util/asserts.ts:16:9)
 - TTY.ref (ext:deno_node/internal_binding/handle_wrap.ts:38:5)
 - ReadStream.ref (node:net:658:20)
 ...

More specifically, it fails here.

The problem seems to be ref() not being implemented in net.Socket. But that confuses me, since std / node / net.ts > Socket#ref suggests that is indeed implemented?

Thoughts?

@littledivy
Copy link
Member Author

@ulken Yes, not all of it is implemented yet. In this case, Socket#ref for TTY handles is unimplemented.

Can you open an issue? Thanks

@ulken
Copy link

ulken commented Nov 15, 2023 •
edited
Loading

@littledivy sure thing. I suppose this is part of the issue?

During the implementation of node:tty we had to make a tradeoff wherein tty.ReadStream and tty.WriteStream were made to extend a Duplex instead of a net.Socket because it introduced circular dependencies at snapshot-time

I'm still confused by the docs I referenced above, it seems to suggest it's indeed implemented. How am I supposed to read it?

@ulken ulken mentioned this issue Nov 15, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@littledivy littledivy

Labels
node compat
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: implement node:tty littledivy/deno
2 participants
@ulken @littledivy