Remove the web-streams-polyfill

[nikithauc]

Concerns:

• I am currently encountering this compilation issue

import { ReadableStream } from 'web-streams-polyfill/es2018';
const request : RequestInit = { body: new ReadableStream()};

Error:

Type 'ReadableStream<any>' is not assignable to type 'BodyInit | null | undefined'.
  Property 'forEach' is missing in type 'import("C:/Work/Projects/SDKS/kiota/abstractions/typescript/node_modules/web-streams-polyfill/dist/types/polyfill").ReadableStream<any>' but required in type 'ReadableStream<any>'.ts(2322)
lib.dom.d.ts(11986, 5): 'forEach' is declared here.

• node-fetch expects a NodeJS.Readable stream as the Request and Response content. https://github.com/node-fetch/node-fetch#bodybody. Example of a conflict error:

1. cross-fetch exports the node-fetch implementation of fetch when the environment is node (or a user using node-fetch library explicitly)
2. The node-fetch Body expects a NodeJS.Readable class. This will clash as the web-streams-polyfill relies on the Web Stream API which is different from node.

Solution:

• Rely on the native default Stream implementation. That is:
  Browser : https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream
  NodeJS : https://nodejs.org/api/stream.html#stream_readable_streams

This is a cleaner approach and working with native implementations gives more flexibility for development.

For example, a node user will more commonly work with the node stream classes and not need the web polyfills. Relying on the native implementation saving user efforts to convert the from web to node definitions.

User's can use polyfill on need basis. We avoid bugs caused by the polyfill. Also, if the polyfill is outdated then native implementation updates are not in sync.

https://www.w3.org/2001/tag/doc/polyfills/#use-native-implementations-when-available

AB#11303

[nikithauc]

Adding new type contentType = NodeJS.ReadableStream | ReadableStreamin abstractionutils`;

RequestInformation.content: contentType;

[baywet]

Thanks, that's really valuable feedback. I had added this polyfill out of old habbit, but data shows we can safely remove it. Would you mind taking this on please?

[baywet]

Also, the polyfill is present in abstrations, serialization/json, http/fetch.

[baywet]

resuming the conversation here since we've made a few "discoveries" in the PR linked above.
my arguments for using this polyfill (or as a ponyfill) were:

• support for the few browsers that still don't support it
• cross compatibility between node and browser environments

it seems we went from that to a union type between the two types (node & browser) first. Which caused type leaking issues across environments

it then seems we went to a empty interface instead to avoid the leakage.

Does this resume the progression well on this issue? Am I missing anything here?

Now the down side of using an empty interface is that now we have to maintain code to understand which runtime are we in, and deal with particularities of imports, instantiation, and potential API surface differences. Which adds a lot of complexity to our code.

Hence my suggestion: let's keep web streams, but use is as a ponyfill, not a polyfill, so globals are not impacted. The main draw back here is that we might have compatibility issues with the fetch library we're targeting, but that can be patched in the request adapter, which limits the impact to a single class, instead of leaking runtime management code all over the place. Any issue with that approach I'm missing?

[nikithauc]

@baywet

The main drawback of using the webstream polyfill ReadableStream is that it is incompatible with the NodeJS.Stream Readable class.

• For a fetch user or any node user working with stream uploads will have to convert everytime. Example how to use with node-fetch? MattiasBuelens/web-streams-polyfill#48. Most request libraries rely on native Stream implementations. (Axios, undici and node-fetch use NodeJS.Stream)

• We still need to be aware of the runtime enviroment when we use the polyfill and convert to node stream. It does not get rid of the API differences.

• We are not leaking runtime management. Separating the implementations in two files and specifying the differences in the package.json - browser config is the standard approach. browser field spec for package.json

• Instantiation does not differ for a node or browser user. A node user simply imports the package and writes code as is. For the browser user, the bundler takes care of the browser config and does not change the instantiation process of the library. The bundler automatically picks up the code.
  What is the aspect of instantiation that you are concerned about?

• Also this polyfill is considered heavy. Reduce the size of dependencies , Polyfill ReadableStream sindresorhus/ky-universal#6 (comment)
  On a side note, we should consider the package size and the number of dependencies we take in because of the infamous node_modules sizes and depth conversations.

• The empty interface is more like a marker interface because we lack a node browser resolution in TypeScript. Using a polyfill which is incompatible with Node implementation will not reflect well with our isomorphic story.

• I would prefer a polyfill or a ponyfill if we were an application. However, as a library author I prefer native implementations to make the library more authentic and take advantage of the environments we support.

[baywet]

too bad the whatwg stream spec implementation is still at an experimental stage, it'd go a long way to solve our issues here.

instead of driving things with an empty interface, could we:

• split the request information like you suggested, but in 3 component:
  1. Request information base, contains almost everything, except the platform specific stuff
  2. Request information (node), contains additional platform specific properties (content, typed of node stream,...), inherits the first one
  3. Request information (browser), contains additional platform specific properties (content, typed of readable stream,...), inherits the first one
• use the browser and node fields in package.json to instruct bundlers to target the right "version" (you've already done a lot of the work)
• take the same approach for the http request adapter, each version would downcast request information to the matching version (if necessary)

What I want to avoid is:

• empty interfaces, they are mostly confusing to people and break compile time validation
• code duplication, it adds a lot of burden on us

[MIchaelMainer]

@baywet @nikithauc

Can you frame the discussion in terms of the kiota maintainers, client developer, and end user developers? This clarification would be really helpful to understand design considerations like this since I think the concerns shared by both of you fall under different aspects of this work. I'd like to understand what are the costs and benefits of this proposed change for each of the personas above. Then it would be easier to determine what course to make.

For example, as I understand it (very basic and I'm confident that I'm missing nuance), @baywet is optimizing for kiota maintainers so that we don't have to manage code duplication and having marker interfaces. @nikithauc is optimizing for client developer and end user developer as they won't need to handle the conversion between the node stream API and web stream API.

Can you both please frame the costs and benefits in terms of these personas? There are burden tradeoffs for the many options here and we should be clear who will benefit and be burdened with each option.

edited

[darrelmiller]

Adding new type contentType = NodeJS.ReadableStream | ReadableStreamin abstractionutils`;

@nikithauc Please don't create a type called ContentType that is type stream. That is very confusing. ContentType is the name of a header that is a string.

[darrelmiller]

@baywet This https://caniuse.com/?search=ReadableStream claims 95% support for ReadableStream.

@nikithauc I agree that 7MB for a polyfill for streams is heavy. Ridiculously so, in my opinion.

I agree that empty interfaces are problematic. Especially for JS newbs like me. I like the idea of derived types per platform.

[nikithauc]

I am currently investigating #841. I am getting a bad request because of the ReadableStream content, that is, the body of the request. Finding out the right way to set the content before discussing more on this issue.

Adding new type contentType = NodeJS.ReadableStream | ReadableStreamin abstractionutils`;

@nikithauc Please don't create a type called ContentType that is type stream. That is very confusing. ContentType is the name of a header that is a string.

This was an alternative considered. Not implementing here. But thank you for bringing up the naming confusion point!

[baywet]

To extend on the conversation:

• The upcoming version of the polyfill is back to a couple of Kbs
• node-fetch is planning to take a dependency on this new version

This would mean we have an easy and isomorphic solution for streams. We'd be left with fetch, for which we could either take a dependency on an isomorphic library (should they chose to upgrade), or limit the impact of switch/shipping variants to http client/fetch request adapter in the http lib.

[nikithauc]

Removed the polyfill in #991.