[@azure-rest/ai-inference] incorrect type when using streaming

[sinedied]

• Package Name: @azure-rest/ai-inference
• Package Version: 1.0.0-beta.5
• Operating system: MacOS
• nodejs
  • version: v22.13.1
• browser
  • name/version:
• typescript
  • version: 5.8.2
• Is the bug related to documentation in
  • README.md
  • source code documentation
  • SDK API docs on https://learn.microsoft.com

Describe the bug
Having to force-cast types like here:

const stream = responseStream.body as IncomingMessage;

when using streaming feels off, it would be best to fix either .asNodeStream() or .createSseStream() method to make them compatible and not force a fix onto users (again, strict linters rules can forbid casting at all).

I'm not sure when the limitation comes from, as the object are compatible with each other so it means that the type definition in one of these two method is wrong.

Expected behavior
Not having to force-cast the type of the stream body, types should be compatible out of the box.

[deyaaeldeen]

The reason createSseStream takes an IncomingMessage is that it has a cancel method that is called in cancellations scenarios. NodeJS.ReadableStream on the other hand doesn't have such API.

Is it possible to update the asNodeStream method to return IncomingMessage? stream cancellation is an important scenario and it is supported by the stream returned by the other method asBrowserStream. /cc @mpodwysocki @jeremymeng @timovv

[jeremymeng]

Is it possible to update the asNodeStream method to return IncomingMessage?

The readableStreamBody on the body may be transformed already (for example, progress report). @joheredi do you know whether that is possible with RLC?

azure-sdk-for-js/sdk/core/core-rest-pipeline/src/nodeHttpClient.ts

Lines 165 to 175 in d18dd5e

	responseStream = shouldDecompress ? getDecodedResponseStream(res, headers) : res;
	const onDownloadProgress = request.onDownloadProgress;
	if (onDownloadProgress) {
	const downloadReportStream = new ReportTransform(onDownloadProgress);
	downloadReportStream.on("error", (e) => {
	logger.error("Error in download progress", e);
	});
	responseStream.pipe(downloadReportStream);
	responseStream = downloadReportStream;
	}

[deyaaeldeen]

@jeremymeng, Great callout! @azure-rest/core-client supports progress reports so yes, we should consider them supported. I think there are a couple candidates to be the asNodeStream's updated return type:

• Readable, which supports the cancellation API and is parent of both http.IncomingMessage and Transform. The downside is that we are exposing NodeJS class type in our APIs which we have been trying not to for some time now.
• A new interface that extends NodeJS.ReadableStream with a destroy method.

createSseStream should also take either of these as input but that should be a straightforward overload addition.

[timovv]

As an alternative to updating the return type of asNodeStream would it be possible to add NodeJS.ReadableStream as an accepted input for createSseStream and feature-detect for destroy in core-sse?

[deyaaeldeen]

@timovv I hate that type tbh and don't want it to spread. Generally, we should give the user the ability to cancel the stream which is useful in cases where the consumer is different from the one made the request.

[timovv]

I just feel that it's less risky than forcing through the other approach which has already caused build failures. Like it or not, the type is there and it's going to be hard to get rid of without a breaking change (we can do it for unbranded though!)

[deyaaeldeen]

The change is not breaking. The build failure is because of multiple copies of core being imported by the test. Are we supporting scenarios that allow multiple different copies living alongside each other?

[xirzec]

The change is not breaking. The build failure is because of multiple copies of core being imported by the test. Are we supporting scenarios that allow multiple different copies living alongside each other?

Technically it's breaking because in PipelinePolicy we return Promise<PipelineResponse> and we're adding a new required parameter to an output type that a consumer must implement. We ran into similar problems when we tried to extend the old FormData primitive in core not that long ago to handle multipart.

While our guidance is to avoid mix and matching versions of core, sometimes it happens, like say:

1. We introduce a small breaking change to core and major version it
2. We update SDK A to use the new core and release that version
3. We update SDK B to use the new core, but haven't released a new version yet (maybe they're in the midst of a service API update)
4. A customer depends on SDK A and B in their application, but now there are two different versions of core pulled in.

Really, the types for node are wrong and should reflect this API as streams have had support for destroy since 8.0: https://nodejs.org/docs/latest/api/stream.html#readabledestroyerror

It looks like someone tried to fix this back in 2017 but ran into issues (probably from other places that reference the old type): DefinitelyTyped/DefinitelyTyped#17977

As a stopgap solution, could we make it optional? It forces us to write ?.destroy() instead of .destroy() but at least there's no casting involved. @deyaaeldeen thoughts?