hapi icon indicating copy to clipboard operation
hapi copied to clipboard

Only support native ReadableStream streams

Open kanongil opened this issue 3 years ago • 3 comments

Support plan

  • is this issue currently blocking your project? (yes/no): no
  • is this issue affecting a production system? (yes/no): no

Context

  • node version: 14+
  • module version: 20.x
  • environment (e.g. node, browser, native):
  • used with (e.g. hapi application, another framework, standalone, ...):
  • any other relevant information:

Handlers currently support returning any ReadableStream, even v1 streams from the readable-stream module. However using such legacy streams is incompatible with modern streams and requires very careful integration to support.

What problem are you trying to solve?

Simpler, more robust internal stream handling. Eg. we can rely on all streams having the .destroy() method and remove tests like: https://github.com/hapijs/hapi/blob/18495f785d602ee23bb01c6dccebb5297e731d7b/test/transmit.js#L1465

Do you have a new or modified API suggestion to solve the problem?

Drop node v12 support (#4279) and fail when the returned stream matches value instanceof Stream && !(value instanceof ReadableStream). The current detection logic relies on duck typing.

One drawback is that we increase the reliance on the node runtime, which means that the Chrome "support" as introduced in #3946 will be gone (unless they find a way to properly polyfill ReadableStream). I don't know why this was patched in, since we don't do any kind of testing on that platform.

For a more graceful approach, we could pipe() legacy streams through a Passthrough stream when returned from a handler. Then internal processing could still rely on the stream being native.

kanongil avatar Oct 01 '21 10:10 kanongil

Thanks @kanongil for bringing this up. Overall this seems like a good idea, I'm not sure though regarding breaking the support for the Chrome use case. Reading through the PR you mentioned their use case seemed legitimate. FWIU if we pipe the legacy streams through a Passthrough stream we'd be able to keep the Chrome support?

Nargonath avatar Oct 01 '21 15:10 Nargonath

This will still break this specific Chrome support, since it most likely polyfills the internal ReadableStream using the readable-stream module, which is outdated and does not expose all modern APIs. We would likely query properties that aren't there.

kanongil avatar Oct 01 '21 17:10 kanongil

This smells like the kind of change I would have made. Not sure what's the "Chrome support" use case...

hueniverse avatar Feb 17 '22 07:02 hueniverse

Resolved with v21 https://github.com/hapijs/hapi/issues/4386

devinivy avatar Nov 07 '22 14:11 devinivy