feat(std/node/process)-stdin-stdout-stderr

[JayHelton]

Description

Adding more backwards compatibility stderr, stdin, stdout for the node/process module'

References

• #7101

[JayHelton]

@ry Id love your thoughts on the preferred way to add these in, or if what i have falls in line with current codebase practices

[ry]

This looks like a good start

[JayHelton]

Heavy WIP branch, still needs lots of testing

[JayHelton]

@ry This is ready for a review round when you have time!

[JayHelton]

Weird. Tests work locally for me. investigating

test process.stdin ... ok (1ms)
test process.stdout ... ok (1ms)
test process.stderr ... ok (1ms)

[balupton]

Could be that the rid changed from definition time to check time on the CI environment.

Perhaps try this way instead:

  stderr: class extends Deno.stderr {
     get isTTY(): boolean {
       return Deno.isatty(this.rid);
     },
   },

Tests for .write and .pipe may also be good, as they are common.

[balupton]

Perhaps try this way instead:

  stderr: class extends Deno.stderr {
     get isTTY(): boolean {
       return Deno.isatty(this.rid);
     },
   },

Actually, that won't work, as it would need to be instantiated.

[balupton]

Maybe something like this using Proxies

  stderr: new Proxy(Deno.stderr, {
     get: function(target, prop, receiver) {
       if ( prop === 'isTTY' ) return Deno.isatty.bind(Deno, this.rid);
       return Reflect.get(...arguments);
     },
   },

[ry]

@JayHelton Any idea why this is failing? Seems to be the related to the improvements you're making.

[JayHelton]

@JayHelton Any idea why this is failing? Seems to be the related to the improvements you're making.

@ry I know whats causing the failure, but not quite sure on the cause of the cause.

The assertion on the isTTY getters are failing for the CI tests, but pass for me locally.

From the ubuntu ci logs with a console.log I added

test process.stdin ... { rid: 0, isTTY: false }
FAILED (0ms)
test process.stdout ... { rid: 1, isTTY: false }
FAILED (0ms)
test process.stderr ... { rid: 2, isTTY: false }
FAILED (1ms)

When i run cargo test std_tests from my mac:

test process.stdin ... { rid: 0, isTTY: true }
ok (3ms)
test process.stdout ... { rid: 1, isTTY: true }
ok (3ms)
test process.stderr ... { rid: 2, isTTY: true }
ok (4ms)

[JayHelton]

Im going to try the above suggestion to Proxy and see if that will achieve more consistent results

[JayHelton]

@ry before i dive deeper into debugging, i wanted to ask you this.

Deno.isatty(Deno.stdin.rid) is evaluating as false on the ubuntu image and im not too sure why. I was under the assumption that stdin should be true. Is that a false assumption?

[JayHelton]

The bug is still around on the ubuntu image and I havent been able to prove what the issue is.

Stdin, Stdout, and Stderr are not recognized as TTYs in the Ubuntu CI, but they are with everything ive tried locally (OSX).

Its not just the node/process std impls, Deno.isatty(Deno.stdin.rid) is also showing as false in CI (again, not locally).

The rid does not change either, as stdin, out, and err are instantiated with rids 0,1, and 2 (consistent with the file descriptors)

In the rust operation, there is a specific block at line 242 in tty.rs that is for unix, and then right above that is windows.

I dont see anything special for linux, so im assuming that on linux is either matching on an error Err(StreamResource::Stdin(..)) or hitting the wildcard which just marks it as false.

I dont have a ubuntu box to test this unfortunately. Any suggestions or ideas?

@ry @nayeemrmn

[ry]

@JayHelton It's probably because the stdio aren't TTYs in the CI scenario. In order to properly test these values, we'd need to setup a PTY... it's not immediately clear to me how we would do that - since Deno doesn't yet have bindings for that...

[JayHelton]

@JayHelton It's probably because the stdio aren't TTYs in the CI scenario. In order to properly test these values, we'd need to setup a PTY... it's not immediately clear to me how we would do that - since Deno doesn't yet have bindings for that...

This makes total sense why im seeing what im seeing.

In the mean time, would you accept a TODO with these isTTY assertions comment out, referencing the above?
If not, should we shelve this for a different time? @ry

[JayHelton]

Additionally, I can start looking into how we can start supporting PTY, though there is a big bag of grey area that i would have to color in

[ry]

In Node there isn't a process.stdin.rid

[JayHelton]

Okay! I’ll get peep everything on node.process and replicate it more explicitly instead of spreading denos stdio.

[ry]

Yes leaving a TODO is fine

[caspervonb]

Be explicit about the properties you want to expose here, this is very brittle and can break anytime additions or changes are made to Deno.stderr (Hasn't happened yet but theoretically it can happen).

[caspervonb]

Ditto

[caspervonb]

Ditto.

[JayHelton]

LMK if this is a bit more inline with what would be safe! What is the denomatic way to port over a method that does not return a promise? The stdout and err both have a write method i was going to add on the objects, but the Deno methods return a promise. It doesnt seem very backwards compatible if they both return a promise

[balupton]

Just to make it explicit, these Node.js methods are needed for any substantial compat.

• *.on
• stdout.pipe
• stderr.pipe
• stdout.write
• stderr.write
• stdin.read

[JayHelton]

Hey all, ive spent time researching the Node implementations of the methods mentioned in previous comments on this PR. I dont know the best way to implement these methods in a way that respects nodes callback based architecture. I am not confident Ill be able to find and implement those solutions without a little pairing and more time. I would recommend reviewing as is, fixing for the specific use case in the original ticket, and then we can add more support as needed.

Additionally, if anyone wants to grab this branch and work on it as well, definitely open to that! I am looking at a busy next few weeks and would prefer if this did not get too stale.

[ry]

LGTM - thank you - sorry for the delay!