fix: Special case ReadableStream type to mean Node.js ReadableStream

[niik]

The StreamProtocolResponse.data property is supposed to be a Node.js readable stream.

Unfortunately the ReadableStream type is ambiguous between the Node.js ReadableStream and the ReadableStream interface defined in the web Streams standard and since the web stream standard is part of the standard lib in TS it'll choose that.

This PR special cases the ReadableStream type name to qualify it with the Node.js namespace.

I wasn't sure what the best approach here would be, special case it in the definition generator or update the documentation to be explicit about the type.

I'm happy to drop this and PR a change to the docs if you'd like.

Diff of generated definitions before/after this change

--- electron.d.ts       2018-05-09 14:42:49.000000000 +0200
+++ electron-niik.d.ts  2018-05-09 14:48:05.000000000 +0200
@@ -3468,7 +3468,7 @@
      * Same as protocol.registerStreamProtocol, except that it replaces an existing
      * protocol handler.
      */
-    interceptStreamProtocol(scheme: string, handler: (request: InterceptStreamProtocolRequest, callback: (stream?: ReadableStream | StreamProtocolResponse) => void) => void, completion?: (error: Error) => void): void;
+    interceptStreamProtocol(scheme: string, handler: (request: InterceptStreamProtocolRequest, callback: (stream?: NodeJS.ReadableStream | StreamProtocolResponse) => void) => void, completion?: (error: Error) => void): void;
     /**
      * Intercepts scheme protocol and uses handler as the protocol's new handler which
      * sends a String as a response.
@@ -3537,7 +3537,7 @@
      * that implements the readable stream API (emits data/end/error events). For
      * example, here's how a file could be returned:
      */
-    registerStreamProtocol(scheme: string, handler: (request: RegisterStreamProtocolRequest, callback: (stream?: ReadableStream | StreamProtocolResponse) => void) => void, completion?: (error: Error) => void): void;
+    registerStreamProtocol(scheme: string, handler: (request: RegisterStreamProtocolRequest, callback: (stream?: NodeJS.ReadableStream | StreamProtocolResponse) => void) => void, completion?: (error: Error) => void): void;
     /**
      * Registers a protocol of scheme that will send a String as a response. The usage
      * is the same with registerFileProtocol, except that the callback should be called
@@ -3951,7 +3951,7 @@
     /**
      * A Node.js readable stream representing the response body
      */
-    data: ReadableStream;
+    data: NodeJS.ReadableStream;
     /**
      * An object containing the response headers
      */

cc @zeke

[ckerr]

Thank you for finding this and proposing a fix!

I like your second offer better, dropping this PR and fixing the docs directly, since changing a few lines of docs is better long-term than adding more special cases to your typescript definitions.

Closing this PR because of that rationale, but I'd be happy to approve & merge a PR on the docs themselves. Thank you!

[ckerr]

Reopening after talking it over with @MarshallOfSound 😃 -- let's do it this way

[niik]

Reopening after talking it over with @MarshallOfSound 😃 -- let's do it this way

Cool. Turns out that there was a bit more to it than "changing a few lines of docs" 😛. First I found that I had to modify the parser in electron-docs-linter to support qualified type names. Once that was done I found a few discrepancies in the docs themselves on master that caused the electron-typescript-definitions build to fail and finally there was a test case that was failing due to electron/electron#12782.

Let me know which ones (if any) of these branches you'd want me to upstream. I'd imagine you'd want the docs changes (minus the qualified type on ReadableStream) and the failing test at least

[MarshallOfSound]

@niik

I'd imagine you'd want the docs changes (minus the qualified type on ReadableStream) and the failing test at least

Yup 😀That would be awesome 👍

[zeke]

Looks good to me. Thanks, @niik 👍

[niik]

Yup 😀That would be awesome 👍

Opened electron/electron#12899 and #101

[niik]

Hey friends, I forgot about this PR until I tried to use the API again and hit the same type error. @ckerr Is there anything more you need from me on this or are we good to merge?

[zeke]

I prepended fix: to the PR title to make sure this triggers a semantic release.

semantic-pull-requests is installed on this repo, but this PR predates it so the status check is not showing up.

To whomever merges this PR, please squash. 🎃