[Bug]: registerStreamProtocol does not work with native fetch Response.body

[Prinzhorn]

Preflight Checklist

• I have read the Contributing Guidelines for this project.
• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the issue tracker for a bug report that matches the one I want to file, without success.

Electron Version

23.0.0

What operating system are you using?

Ubuntu

Operating System Version

Linux alex-NUC11PAHi7 5.19.0-31-generic #32-Ubuntu SMP PREEMPT_DYNAMIC Fri Jan 20 15:20:08 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

What arch are you using?

x64

Last Known Working Electron version

N/A

Expected Behavior

Returning Response.body of the native fetch API from registerStreamProtocol should stream the thing

Actual Behavior

Apparently Response.body of the fetch API is supposed to be a ReadableStream. But returning it from registerStreamProtocol does not work (ERR_FAILED).

Testcase Gist URL

https://gist.github.com/5ba47c8cd5e44d19bb97bb2100b32983

Additional Information

The Fiddle is based on the original example from the docs https://www.electronjs.org/docs/latest/api/protocol#protocolregisterstreamprotocolscheme-handler

Commenting out the line and using data: createStream('<h5>Response</h5>') streams the things.

Maybe a better error message than ERR_FAILED would also be noice.

[dsanders11]

I'm not sure this is necessarily a bug, more confusing due to Node.js changes. Historically Node.js only had NodeJS.ReadableStream, which is what the Electron documentation is referring to, and supports. Now with the addition of the fetch API in Node.js, they have added support for Web Streams and the ReadableStream type which fetch.body refers to is the Web Stream ReadableStream.

It looks like the Node.js docs now refer to NodeJS.ReadableStream as just Readable. You can use stream.Readable.fromWeb to make response.body from fetch work with registerStreamProtocol. Abbreviated updated version of the Fiddle code:

const { PassThrough, Readable } = require('stream')

[...]

data: Readable.fromWeb(response.body)

[Prinzhorn]

Indeed, that is confusing (if you also throw ReadStream into the mix, which is a ReadableStream). Thank you very much, it's working now as intended.

I don't think Electron needs to necessarily transparently handle this for me. Maybe this has now turned into a feature request for:

• Improve the error message / dx of protocol. If you pass anything as data that is not what Electron expects, you just get a log that the request failed with ERR_FAILED. Sth. like "data was not a Buffer/ReadableStream" etc. would go a long way.
• Add the Readable.fromWeb example to the docs. Since global fetch is now in Electron as well I can just assume this might happen not just to me in the future. I feel like fetch + registerStreamProtocol is way more powerful than registerHttpProtocol (which gives limited control).

[MarshallOfSound]

Add the Readable.fromWeb example to the docs. Since global fetch is now in Electron as well I can just assume this might happen not just to me in the future. I feel like fetch + registerStreamProtocol is way more powerful than registerHttpProtocol (which gives limited control).

This is an interesting case but I'd make the IMO compelling case for Electron to disable / replace the global fetch in the main process with either our own implementation --> #36733 or by just removing it from the global scope.

It has a giant footgun in that it uses Node's network stack instead of Chromiums which means requests don't route through webRequest, follow proxies correctly, use the same cert verifier, etc. etc.

cc @nornagon Thoughts on using net.fetch to replace globalThis.fetch?

[Prinzhorn]

It has a giant footgun in that it uses Node's network stack instead of Chromiums

This goes both ways. "Replacing fetch with our own implementation has a giant footgun of people expecting it to behave like native fetch" (#26593).

or by just removing it from the global scope.

It will probably break npm packages that build on global fetch, increasing the gap between Electron and Node.js. I was hoping the list of differences would shrink, not increase.

In the context of protocol I can see the problem, but not for fetch in general. What about registerFetchProtocol that gives you a fetch as argument? And you can literally return fetch() and it works with the Response promise? It could also pass along an AbortController or signal so we can properly cancel intercepted requests when the underlying request dies.

In any case, if you go down the route of replacing it please give us sth. like the original-fs import so that I can get the actual fetch. I explicitly do not want Chromium's network stack for my use-case. There's also an argument to be made that Electron does not patch the native net module but instead has it's own net module. If I understand correctly you can/will be able to import Electron's net.fetch if you need to.

There could also be a flag to re-enable native fetch but default to Electron's version. As long as I can keep using native global fetch in some way.

Also what about Worker threads? Right now I treat my server worker like any Node.js app (and Workers do behave different than the main process). Would you also replace fetch there? That would again break perfectly functioning Node.js apps.

[MarshallOfSound]

Just to be super clear, my comment was spitballing. Not a recommended course of action. The point I was trying to make was globalThis.fetch being different in the main process and the renderer is only gonna cause issues for us in the long run, we should get ahead of it now somehow the three options are "replace it with an API compatible version that uses chromium net stack" or "kill it in the main process" (which IMO is fine until Node 16 is not LTS but not my preference by any shot) or "document it very obviously as a footgun".

We could also leave it alone but IMO that's not viable, random node modules will use globalThis.fetch as a fallback and all network connections made in an Electron app should go through the Chromium network stack, not through the nodejs one.

I explicitly do not want Chromium's network stack for my use-case

Details? I'm not aware of such usecases

[Prinzhorn]

Thanks for clarifying.

all network connections made in an Electron app should go through the Chromium network stack, not through the nodejs one.

Could you point to documentation about this? I don't understand why I would want that? But if that's something that needs to be done for some technical reason then I'd love to know about this.

Details? I'm not aware of such usecases

I've never though about it in detail, but for me Electron is a client server architecture. And my server is a literal web server running in a worker thread that serves the content for the renderer. As such it doesn't make sense to use Chromium's network stack (for requests I make within the server to the internet) since this is a Node.js application. I also don't know enough about Chromium's network stack to "trust" it. It's a magic black box optimized for web browsing whereas I've been working with the Node.js network stack for over ten years. For example I don't know if Chromium's network stack would transparently handle cookies for me or if it would cache things. And I don't feel like digging into these things since I know that the Node.js stack won't do anything like that without my consent. Chromium's network stack also has limitations (coming from the fact that it is made for web browsing) such as not having full control over all headers. Also isn't it limited to HTTP(S) only? How would I do anything else with it's network stack?

If my renderer needs to use a proxy and do other web browser related things, then I don't see how this has anything to do with my server needing a proxy. These are completely separate concerns?

[nornagon]

cc @nornagon Thoughts on using net.fetch to replace globalThis.fetch?

I think this is a bad idea and we shouldn't do it. There are good reasons for apps to want to use Node's networking stack, and it would be surprising for globalThis.fetch() to function differently in Electron main vs Node. If an app wants that behavior, it can quite straightforwardly opt-in with globalThis.fetch = net.fetch; an opt-out strategy would be more complex to implement.

As for this bug/feature request, I have plans to significantly improve the protocol module in the near future (see electron/governance#368), which should hopefully include this sort of improvement, so I'm going to close this out.