Interface 'SrcOptions' incorrectly extends interface 'Options'

[j-oliveras]

Steps to reproduce.

1. Donwload this repository and npm install.
2. Update typescript version npm i typescript@^2.0.10 -D.
3. Update node typings dependency typings i env~node -SG.
4. npm run build and npm run test.
5. Test fails with:

test/bundle.d.ts(503,11): error TS2430: Interface 'SrcOptions' incorrectly extends interface 'Options'.
  Types of property 'read' are incompatible.
    Type 'boolean' is not assignable to type '(size?: number) => any'.

This is the line that causes the error.

This is because SrcOptions extends through.Options that extends stream.DuplexOptions that extends ReadableOptions that defines read property as read?: (size?: number) => any;.

[felixfbecker]

Does this only happen when you update to TS2 and the node typings?
Anyway, PR welcome

[j-oliveras]

Update typescript is needed to avoid compile errors with new versions of node typings (new versions of node typings adds support for non nullable types).

The problem is that after this commit (2016-11-05) the read property of node ReadableOptions interface (parent type of SrcOptions) conflicts with SrcOptions read property type.

[felixfbecker]

@blakeembrey I can't find the read option in the documentation, could you comment on this?

[blakeembrey]

@felixfbecker See https://nodejs.org/api/stream.html#stream_implementing_a_readable_stream for the read method. It's possible the node.js definition incorrectly extends the read and write options for duplex and readable streams, it's not in the docs and was just legacy I kept there. Also, through2 should probably be updated to extend stream.TransformOptions instead of DuplexOptions (though irrelevant here).

[felixfbecker]

@blakeembrey Afaik you should implement these when subclassing Readable, not pass them as an option when instantiating.

[blakeembrey]

Reviewing the node.d.ts is actually (mostly) correct - see https://github.com/nodejs/node/blob/1f45d7aa41291383ce63cca5f80fd2a2a73d7603/lib/_stream_transform.js#L93 and https://github.com/nodejs/node/blob/1f45d7aa41291383ce63cca5f80fd2a2a73d7603/lib/_stream_duplex.js#L27-L28.

[blakeembrey]

@felixfbecker Read the options signature and the "Simplified Constructor approach":

read Implementation for the stream._read() method.

[blakeembrey]

The method you need to implement is actually called _read.

[blakeembrey]

@felixfbecker The code works in production because node.js checks for a function type: https://github.com/nodejs/node/blob/1f45d7aa41291383ce63cca5f80fd2a2a73d7603/lib/_stream_readable.js#L113. If it ever changed to an error, I think vinyl-fs would break here. It's tricky, since all the definitions are 100% correct and vinyl-fs is instead relying on a runtime behaviour of passing a non-function being ignored. What do you think best solution is? If we had subtraction types, you could just subtract .read.

[felixfbecker]

We could do a union type boolean | Function and document as such?

[blakeembrey]

Sure, that seems reasonable. I just realised vinyl-fs actually does implement a workaround for this: https://github.com/gulpjs/vinyl-fs/blob/61c7faabff0dc9f2d4126c6829f44b500964a003/lib/src/index.js#L36-L37.

[felixfbecker]

Hm, then you are right, a difference type would be best for this...

[j-oliveras]

This don't work.

• boolean | Function change the error to:

test/bundle.d.ts(503,11): error TS2430: Interface 'SrcOptions' incorrectly extends interface 'Options'.
  Types of property 'read' are incompatible.
    Type 'boolean | Function' is not assignable to type '(size?: number) => any'.
      Type 'true' is not assignable to type '(size?: number) => any'.

• boolean | ((size?: number) => any) the error is:

test/bundle.d.ts(503,11): error TS2430: Interface 'SrcOptions' incorrectly extends interface 'Options'.
  Types of property 'read' are incompatible.
    Type 'boolean | ((size?: number) => any)' is not assignable to type '(size?: number) => any'.
      Type 'true' is not assignable to type '(size?: number) => any'.

[andypyrope]

I encountered this issue as well. After some looking through the vinyl-fs code I found out that neither
function through2(options?: through2.Options, transformFunction?: through2.TransformFunction, flushFunction?: through2.FlushCallback): NodeJS.ReadWriteStream;
nor
function ctor(options?: Options, transformFunction?: TransformFunction, flushFunction?: FlushCallback): NodeJS.ReadWriteStream
are used anywhere in it. These are the only calls that reference the Options interface. Therefore there should be no reason for SrcOptions to extend through.Options.

Also, if there really was a reason for it to extend through.Options, then that reason could only be that SrcOptions is passed to a function that accepts through.Options despite it containing a 'read' property that isn't of the expected type. That would be a serious bug that should be fixed in vinyl-fs itself and I doubt this is the case.