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

process.stdout should be WritableStream | tty.WriteStream #19

Open
felixfbecker opened this issue Aug 13, 2016 · 23 comments
Open

process.stdout should be WritableStream | tty.WriteStream #19

felixfbecker opened this issue Aug 13, 2016 · 23 comments

Comments

@felixfbecker
Copy link

felixfbecker commented Aug 13, 2016

From https://nodejs.org/api/tty.html#tty_tty:

When Node.js detects that it is being run inside a text terminal ("TTY") context, the process.stdin will, by default, be initialized as an instance of tty.ReadStream and both process.stdout and process.stderr will, by default be instances of tty.WriteStream. The preferred method of determining whether Node.js is being run within a TTY context is to check that the value of the process.stdout.isTTY property is true:

$ node -p -e "Boolean(process.stdout.isTTY)"
true
$ node -p -e "Boolean(process.stdout.isTTY)" | cat
false

It's currently not possible to check process.stdout.isTTY because it is only typed as WritableStream. It should be WritableStream | tty.WriteStream.

The alternative check process.stdout instanceof tty.WriteStream is not possible either, because tty only exports interfaces, not the classes / constructors.

The same applies to process.stdin/tty.ReadStream

@blakeembrey
Copy link
Member

Seems like a good change. I think both the read and write streams should just be updated to the tty interfaces and not use streams directly.

@felixfbecker
Copy link
Author

They are totally different classes though. tty streams inherit from net.Socket

@blakeembrey
Copy link
Member

blakeembrey commented Aug 13, 2016

Ok, then what I should have said is to create new interfaces for this. I don't think you'd want a union here, TypeScript would still complain it doesn't exist until you do a type assertion then.

Edit: Sorry, you're right. Read the example wrong. You're definitely welcome to do a PR to quickly update as well 😄

@blakeembrey
Copy link
Member

Ok, so it turns out this is a real PITA to fix - any ideas? The issue is that the tty, net, stream, etc. are all modules by process it global. I wish there was a nice way in TypeScript to mix global with modules (like declare var process: Process from 'process' or something).

@felixfbecker
Copy link
Author

@blakeembrey shouldn't this work?

import {Process} from 'process';
declare const process: Process;

Assuming Process is an interface

@blakeembrey
Copy link
Member

blakeembrey commented Aug 24, 2016

No. You can't do that because the second you put import at the top-level, it makes the definition an external module format and not global. That's sadly just the rules TypeScript uses.

@felixfbecker
Copy link
Author

And the other way around?

declare namespace NodeJS {
  export interface Process {}
}  
declare module 'process' {
  type Process = NodeJS.Process;
  export {Process};
}

@blakeembrey
Copy link
Member

blakeembrey commented Aug 24, 2016

Yes, that's possible - but it requires moving all of stream, net, tty (just the pieces in use here) into the global NodeJS scope. It's actually the trick that's currently in use today, I just didn't want to make that change right away because it is a huge change.

@felixfbecker
Copy link
Author

felixfbecker commented Aug 24, 2016

And you cannot use import inside a namespace, right? I think that is the fundamental limitation here.

@blakeembrey
Copy link
Member

Correct 👍 But this isn't just in the namespace, process is a variable that needs declaring at the top-level. There's a few fundamental limitations here. I'm not sure, but maybe we can propose a change in TypeScript to curb this because it definitely isn't the first time.

My initial thought is a TypeScript directive that'll ensure the file is always considered global or external. /cc @mhegazy @RyanCavanaugh How feasible would it be to allow import at the top-level if there was something to declare this file as a global declaration?

@felixfbecker
Copy link
Author

felixfbecker commented Aug 24, 2016

@blakeembrey It could be a new triple-slash directive.

Alternatively, it would also be enough if we could import inside namespaces. Then we could declare the globals by referencing the type from the namespace.

declare const process: NodeJS.Process;
declare namespace NodeJS {
  export {Process} from 'process';
}

@RyanCavanaugh
Copy link

I don't understand the question -- TypeScript always considers files as global or module depending on presence of top-level import / export declarations. You can use declare global { if you want something to go into the global scope from within a module file.

@felixfbecker
Copy link
Author

So we would change the whole declaration file to a module, but then put the majority of it inside declare global. Interesting! Might make the whole code way more structured because we can put stream stuff inside the actual 'stream' module, tty stuff inside 'tty', process inside 'process' etc.

@felixfbecker
Copy link
Author

@blakeembrey Could we even make individual files for the core node modules that are purely external? And then have one index that imports everything and declares the globals?

@blakeembrey
Copy link
Member

@RyanCavanaugh And that's exactly our problem. We need to share an interface globally declare var process: Process and within modules declare module "process". The pain here comes from the fact that some things have a very big module dependency chain to work (E.g. tty -> net -> stream) which all have to be moved global just to make process work. However, if we could import from a module into the global, this would be possible without polluting the global namespace with a ton of things that don't exist.

@blakeembrey
Copy link
Member

blakeembrey commented Aug 25, 2016

@felixfbecker Unfortunately that's not possible either, because we need to put declare module "stream" so TypeScript can recognise them. So either way, all the files would need to be global.

Using declare global may be possible with 2.0. Might be worth trying but I think the issue here is how TypeScript would treat this file. Since it's now external, I believe the global declaration would only take effect after the externalised node.d.ts module is imported. Is that correct @RyanCavanaugh?

@felixfbecker
Copy link
Author

@RyanCavanaugh I just tried out your suggestion, but it doesn't work:

declare module 'process' {
  export interface Process {}
}
import {Process} from 'process';
declare global {
    const process: Process; // Error: Module augmentation cannot introduce new names in the top level scope.
}

@blakeembrey
Copy link
Member

@felixfbecker I think the other issue with that is that import at the top-level makes the file external, which results in declare module "process" being an augmentation instead of a declaration of the original module. This, in turn, seems to result in errors with "process" not being found to import from to start with. At least on the latest nightly (which 2.0 does fix the module augmentation error you see) occurs with test.d.ts(4,23): error TS2307: Cannot find module 'process'..

@felixfbecker
Copy link
Author

felixfbecker commented Aug 25, 2016

That error can be circumvented by doing:

process.d.ts

export interface Process {}

node.d.ts

import {Process} from './process';
declare global {
    const process: Process; // Error: Module augmentation cannot introduce new names in the top level scope.
}

The error I mentioned remains though.

@blakeembrey
Copy link
Member

@felixfbecker Correct, but then there's no "process" global module declared, only a ./process external module. The error you mentioned is fixed in 2.0.

@blakeembrey
Copy link
Member

blakeembrey commented Aug 25, 2016

The only way around all this may be to combine module files, references and imports. E.g. Write declare "process" {} in it's own file, use it as a reference, then import { Process } from 'process' like your example above.

Edit: However, none of this fixes the concerns mentioned in #19 (comment) because having node.d.ts as an external module probably won't be used how we want it to be.

@felixfbecker
Copy link
Author

@blakeembrey Besides the error that is fixed in TS2.0, I found a solution:

process.d.ts

declare module 'process' {
  export interface Process {}
}

node.d.ts

/// <reference path="./process.d.ts" />
import {Process} from 'process';
declare global {
  declare const process: Process;
}

@blakeembrey
Copy link
Member

Yes, that's the proposal I suggested above. But it does not fix the concern I expressed (also above).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants