-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
fix: implement node:tty #20892
fix: implement node:tty #20892
Conversation
function makeLazyStream<T>(objectFactory: () => T): T { | ||
return new Proxy({}, { | ||
get: function (_, prop, receiver) { | ||
// deno-lint-ignore no-explicit-any | ||
return Reflect.get(objectFactory() as any, prop, receiver); |
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.
This breaks code in _streams.mjs
because of the use of Proxy:
divy@mini /tmp> ~/gh/deno/target/debug/deno run -A npm:nuxi@latest init my-app
[5:25:30 PM] ERROR 'get' on proxy: property '_eventsCount' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value (expected '2' but got '1')
at _addListener (ext:deno_node/_stream.mjs:1884:9)
at Proxy.addListener (ext:deno_node/_stream.mjs:1913:14)
at Proxy.Readable.on (ext:deno_node/_stream.mjs:3275:39)
at Proxy.Readable.pipe (ext:deno_node/_stream.mjs:3177:11)
when its trying to update the eventsCount. I reverted it back to not lazy load the stdin/out/err streams which fixed the issue.
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.
Can you enable some additional compat tests for this PR?
@bartlomieju all tty related compat tests seem to be enabled already |
I'm not sure - eg. I can see |
|
Signed-off-by: Divy Srivastava <[email protected]>
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.
Looks good, some simplification and unsafe removal suggestions.
|
||
#[repr(u32)] | ||
enum HandleType { | ||
#[allow(dead_code)] |
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.
TODO for later work?
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.
Dead code on Windows for TCP and UDP is TODO.
/bench startup_nop_trivial |
startup_nop_trivial
start: id: server: |
Looks like no real startup impact of this change per the benchmark below. Awesome! |
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, nice
@mmastrac The early is_terminal check seems to bring back the hang on Windows. I've reverted back. |
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 too, try to enable some of the Node compat tests that check if the terminal is restored on exit
As a heads up, it was reported that using the latest Deno from |
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)
Fixes #21012
Closes #20855
Fixes #20890
Fixes #20611
Fixes #20336
Fixes
create-svelte
from #17248Fixes more reports here: