node: process.stdin is a stream.Readable and process.stdout is a stream.Writable #20493

dex4er · 2017-10-11T14:46:49Z

New interfaces NodeJS.Readable and NodeJS.Writable have the same properties
as in stream.Readable and stream.Writable classes.

Interface NodeJS.Socket is just a NodeJS.ReadWriteStream. But new
interface NodeJS.TtyStream (TtyWriteStream and TtyReadStream) has
extra properties as in tty.ReadStream and tty.WriteStream.

process.stdin implements NodeJS.TtyReadStream and
process.stdout implements NodeJS.TtyWriteStream.

net.Socket, dgram.Socket, fs.Stats, fs.ReadStream and fs.WriteStream
are classes.

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If adding a new definition:

The package does not provide its own types, and you can not add them.
If this is for an NPM package, match the name. If not, do not conflict with the name of an NPM package.
Create it with dts-gen --dt, not by basing it on an existing project.
tslint.json should be present, and tsconfig.json should have noImplicitAny, noImplicitThis, and strictNullChecks set to true.

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: <>
Increase the version number in the header if appropriate.
If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

If removing a declaration:

If a package was never on DefinitelyTyped, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
Delete the package's directory.
Add it to notNeededPackages.json.

dt-bot · 2017-10-11T14:47:02Z

types/hexo-fs/index.d.ts

to author (@segayuu). Could you review this PR?
👍 or 👎?

types/node/index.d.ts

to authors (@DefinitelyTyped/DefinitelyTyped @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @Flarna @mwiktorczyk @wwwy3y3 @Tyriar @DeividasBakanas @kjin @alvis Microsoft TypeScript (account can't be detected)). Could you review this PR?
👍 or 👎?

typescript-bot · 2017-10-11T15:49:29Z

@dex4er Please fix the failures indicated in the Travis CI log.

dex4er · 2017-10-11T16:44:43Z

Travis died because of current problems in master branch.

This pull request should fix #20496 and #20487

Flarna · 2017-10-11T20:59:58Z

types/node/index.d.ts

@@ -5033,7 +5084,7 @@ declare module "stream" {
            isPaused(): boolean;
            unpipe<T extends NodeJS.WritableStream>(destination?: T): this;
            unshift(chunk: any): void;
-            wrap(oldStream: NodeJS.ReadableStream): Readable;
+            wrap(oldStream: NodeJS.ReadableStream): NodeJS.ReadableStream;


This seems to be not correct. warp method returns a Readable class or even better this.

so maybe better is wrap<T extends NodeJS.ReadableStream>(oldStream: NodeJS.ReadableStream): T

It will break some modules which define wrap method, but it is very easy to fix (maybe 3 modules in DefinitelyTyped repo)

I think it should be wrap(oldStream: NodeJS.ReadableStream): this as this is how it is implemented (and documented).

Flarna · 2017-10-11T21:06:01Z

types/node/index.d.ts

-        openStdin(): Socket;
+        stdout: StdWriteStream;
+        stderr: StdWriteStream;
+        stdin: StdReadStream;


The StdReadStream you defined has a setRawMode() and isRaw like tty.ReadStream.
If you pipe in a file to node process.stdin doesn't have them.

Flarna · 2017-10-11T21:10:04Z

I wonder why there are new types added as NodeJs docu tells that process.stdin is either a net.Socket or a stream.Readable. Can't we use them here instead of creating new interfaces?

dex4er · 2017-10-12T10:37:51Z

@Flarna It would be better to use

stdin: fs.ReadStream | tty.Socket;

but NodeJS are built with interfaces only. So:

stdin: NodeJS.ReadStream | NodeJS.Socket;

but... it is very inconvenient. It would mean that I had to check the instanceof or use duck typing:

console.log(process.stdin.isTTY); // fails because not defined in fs.ReadStream
console.log(process.stdin.path); // fails because not defined in net.Socket

So that was idea behind TtyReadStream: it has properties from both fs.ReadStream and net.Socket, so it is perfectly legal:

console.log(process.stdin.isTTY); // undefined when stdin is fs.ReadStream
console.log(process.stdin.path); // undefined when stdin is net.Socket

Flarna · 2017-10-12T12:59:06Z

With your proposed solution it's valid to write process.stdin.setRawMode(false) but it may crash depending on the type of stream stdin points to.
Hopefully I find some time soon to play around a little and provide some more concrete proposals.

dex4er · 2017-10-12T13:19:56Z

@Flarna Contre-proposition is to really make NodeJS.Process.stdin as fs.ReadStream | tty.ReadStream alternative and then you couldn't do:

if (process.stdin.isTTY) console('this is tty');

and you should replace it with:

if (process.stdin instanceof tty.ReadStream) console('this is tty');

Maybe this is the right way in Typescript and we should not take all patterns from Javascript blindly.

Flarna · 2017-10-12T19:57:54Z

I did a closer look into the various definition of streams. Hopefully I understand your problem and proposal better now.

One issue is that it seems we can't use the "correct" types net.Socket, ... in namespace NodeJS as they are not visible and can't be imported.
Workaround is to define interfaces in NodeJS which look like the classes in stream/fs/net but it seems there got out of sync over time.
I think instead of creating new interfaces Readable and Writeable extending the existing ones (e.a. ReadableStream and WriteableStream it would be better to simply updated the existing ones with the missing properties.
In current definition properties isTTY, columns, rows, isRaw and setRawMode were alredy present but optional. I think they should be kept optional as they are in fact not always there.

Maybe an alternative to keep interfaces in NodeJS and classes in stream/... in sync would be to define the classes already in NodeJS and re-export the type and constructor in stream/... But I'm not sure which side effects this may have.

dex4er · 2017-10-13T00:52:55Z

@Flarna ok so this is another try, after rebase to the newest master:

stream, fs, net and dgram have classes, not interfaces, so instanceof fs.ReadStream is possible.
NodeJS.ReadableStream and WritableStream are simple interfaces so it is possible to write own ReadableStream without extra methods (_read or _write).
NodeJS.ReadStream and WriteStream were used only for process.stdin and process.stdout and now have defined all missing methods so the interface is compatible with stream.readable.

Now most of problems are fixed and changed don't break other modules, except firebird and some of them that reimplemented net.Socket class because of bug in @types/node.

Flarna · 2017-10-13T06:59:06Z

Thanks! Hope this still solves the problems you have seen.
Changes in node look fine to me but I think the changes in firebird should be reviewed by an firebird expert (@karak).

karak · 2017-10-13T08:31:17Z

@Flarna I'm sorry to say I never used the stream and blob APIs. Actually, I just followed the reference document of node-firebird( https://github.com/xdenser/node-firebird-libfbclient#stream-object) about them.

It's author @xdenser could help about the relation between Firebird Streams and NodeJS Streams.

APPEND:
I just walked through the .js and compare it with the new .ts, and the function overrides seem fine, as you may know.

dex4er · 2017-10-13T09:07:25Z

@karak @Flarna I made the changes for firebird based on the current code https://github.com/xdenser/node-firebird-libfbclient/blob/master/firebird.js#L170

fb.Stream extends stream.Stream and implements few methods. Exactly the same as in the typings from this patch.

ghost · 2017-10-16T20:19:08Z

types/hexo-fs/index.d.ts

@@ -426,11 +426,6 @@ export function writeFile(
 */
 export function writeFileSync(path: string, data: any, options?: string | { encoding?: string | null; mode?: string | number; flag?: string }): void;

-// Static classes
-export let Stats: Stats;


This appears to remove an exported variable from an unrelated module?

I can reexport class from fs to get the same behaviour as before

It is now rebased and fixed

…; change interfaces in fs, dgram and net into classes

…tream interface (and Write...)

ghost · 2017-10-17T14:44:16Z

Thanks!

…am.Writable (DefinitelyTyped#20493) * Readable.wrap returns this * Add missing properties to interface ReadableStream and WritableStream; change interfaces in fs, dgram and net into classes * Move all additional properties from ReadableStream interface to ReadStream interface (and Write...) * hexo-fs should re-export classes from 'fs' * process.stdin._destroy is a function

dex4er requested a review from vvakame as a code owner October 11, 2017 14:46

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Oct 11, 2017

dex4er mentioned this pull request Oct 11, 2017

@type/node: process.stdin are neither stream.Readable nor net.Socket and not stream.Readable at all #20487

Closed

4 tasks

Flarna suggested changes Oct 11, 2017

View reviewed changes

dex4er force-pushed the node_streams branch from 7e3356c to 7d3cf30 Compare October 13, 2017 00:44

typescript-bot added The Travis CI build failed and removed The Travis CI build failed labels Oct 13, 2017

Flarna approved these changes Oct 13, 2017

View reviewed changes

typescript-bot added the Author Approved label Oct 13, 2017

ghost reviewed Oct 16, 2017

View reviewed changes

dex4er closed this Oct 17, 2017

dex4er deleted the node_streams branch October 17, 2017 12:51

dex4er restored the node_streams branch October 17, 2017 12:52

dex4er reopened this Oct 17, 2017

dex4er added 3 commits October 17, 2017 14:55

Readable.wrap returns this

6efa3e3

Add missing properties to interface ReadableStream and WritableStream…

1301859

…; change interfaces in fs, dgram and net into classes

Move all additional properties from ReadableStream interface to ReadS…

ee7ce48

…tream interface (and Write...)

dex4er added 2 commits October 17, 2017 15:02

hexo-fs should re-export classes from 'fs'

7b582bb

process.stdin._destroy is a function

56fe408

dex4er force-pushed the node_streams branch from 7d3cf30 to 56fe408 Compare October 17, 2017 13:06

ghost merged commit 8ef0c39 into DefinitelyTyped:master Oct 17, 2017

dex4er deleted the node_streams branch October 23, 2017 12:18

Flarna mentioned this pull request Oct 24, 2017

@types/node - fs.ReadStream incorrectly defined as an interface only #20496

Closed

4 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: process.stdin is a stream.Readable and process.stdout is a stream.Writable #20493

node: process.stdin is a stream.Readable and process.stdout is a stream.Writable #20493

dex4er commented Oct 11, 2017

dt-bot commented Oct 11, 2017

typescript-bot commented Oct 11, 2017

dex4er commented Oct 11, 2017

Flarna Oct 11, 2017

dex4er Oct 12, 2017

Flarna Oct 12, 2017 •

edited

Loading

Flarna Oct 11, 2017

Flarna commented Oct 11, 2017

dex4er commented Oct 12, 2017

Flarna commented Oct 12, 2017

dex4er commented Oct 12, 2017 •

edited

Loading

Flarna commented Oct 12, 2017

dex4er commented Oct 13, 2017

Flarna commented Oct 13, 2017

karak commented Oct 13, 2017 •

edited

Loading

dex4er commented Oct 13, 2017

ghost Oct 16, 2017

dex4er Oct 17, 2017

dex4er Oct 17, 2017

ghost commented Oct 17, 2017

node: process.stdin is a stream.Readable and process.stdout is a stream.Writable #20493

node: process.stdin is a stream.Readable and process.stdout is a stream.Writable #20493

Conversation

dex4er commented Oct 11, 2017

dt-bot commented Oct 11, 2017

typescript-bot commented Oct 11, 2017

dex4er commented Oct 11, 2017

Flarna Oct 11, 2017

Choose a reason for hiding this comment

dex4er Oct 12, 2017

Choose a reason for hiding this comment

Flarna Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Flarna Oct 11, 2017

Choose a reason for hiding this comment

Flarna commented Oct 11, 2017

dex4er commented Oct 12, 2017

Flarna commented Oct 12, 2017

dex4er commented Oct 12, 2017 • edited Loading

Flarna commented Oct 12, 2017

dex4er commented Oct 13, 2017

Flarna commented Oct 13, 2017

karak commented Oct 13, 2017 • edited Loading

dex4er commented Oct 13, 2017

ghost Oct 16, 2017

Choose a reason for hiding this comment

dex4er Oct 17, 2017

Choose a reason for hiding this comment

dex4er Oct 17, 2017

Choose a reason for hiding this comment

ghost commented Oct 17, 2017

Flarna Oct 12, 2017 •

edited

Loading

dex4er commented Oct 12, 2017 •

edited

Loading

karak commented Oct 13, 2017 •

edited

Loading