node-fetch/fetch-blob

require "stream/web" produces warning

zdm opened this issue · 21 comments

zdm commented

Hi,
On node v16 it produces experimental warning.

(node:18876) ExperimentalWarning: stream/web is an experimental featur

Could you please use polyfill until stream/web feature will get stable status?

Also instead of using try/catch for load stream/web you can check process.version and load stream/web if node version is >= 16.5.0.

regseb commented

I have the same warning with Node v16.8.0. The Web Streams API has been added in Node v16.5.0 but is experimental. I think you should use web-streams-polyfill until the API is stable (streams.cjs).

(node:23167) ExperimentalWarning: stream/web is an experimental feature. This feature could change at any time
    at emitExperimentalWarning (node:internal/util:227:11)
    at node:stream/web:7:1
    at NativeModule.compileForInternalLoader (node:internal/bootstrap/loaders:312:7)
    at NativeModule.compileForPublicLoader (node:internal/bootstrap/loaders:252:10)
    at loadNativeModule (node:internal/modules/cjs/helpers:41:9)
    at Function.Module._load (node:internal/modules/cjs/loader:804:15)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:94:18)
    at Object.<anonymous> (/home/regseb/testcase/node_modules/fetch-blob/streams.cjs:7:31)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
zdm commented

stream/web api will be experimental for a year or more, so we are doomed to see this warning everywhere for a long time. ;-(

  • 😄1
  • 😕1
jimmywarting commented

I want to use the stream version developers will probably use cuz they are not interchangeable.
you can't pipe data from web-streams-polyfill to node's whatwg:stream

Also instead of using try/catch for load stream/web you can check process.version and load stream/web if node version is >= 16.5.0.

I also use this pkg on other non-nodejs places like Deno - Deno don't have stuff like blob's backed up by the filesystem. so i would prefer not to use version detection.

Deno for instances already have a global stream api... but i doubt this will be long lived, both Deno and Node's blob currently support async blobs and they have the potential to read data from the disc in a async manner, but non have a way of actually getting a blob from the disc yet.

zdm commented

Could you, please, move stream/web initialization code to the Blob.prototype.stream function?
Like this:

var initialized;

try {
    const { Blob } = require( "buffer" );

    if ( Blob && !Blob.prototype.stream ) {
        Blob.prototype.stream = function name ( params ) {
            if ( !initialized && !globalThis.ReadableStream ) {
                initialized = true;
                try {
                    Object.assign( globalThis, require( "stream/web" ) );
                }
                catch ( error ) {

                    // TODO: Remove when only supporting node >= 16.5.0
                    Object.assign( globalThis, require( "web-streams-polyfill/dist/ponyfill.es2018.js" ) );
                }
            }

            let position = 0;
            const blob = this;

            return new ReadableStream( {
                "type": "bytes",
                async pull ( ctrl ) {
                    const chunk = blob.slice( position, Math.min( blob.size, position + POOL_SIZE ) );
                    const buffer = await chunk.arrayBuffer();
                    position += buffer.byteLength;
                    ctrl.enqueue( new Uint8Array( buffer ) );

                    if ( position === blob.size ) {
                        ctrl.close();
                    }
                },
            } );
        };
    }
}
catch ( error ) {}
  • 👎5
jimmywarting commented

It have been on my mind. we could do something like that.
But we should probably expose a function that can run the initialized part

the stream are needed here also:

fetch-blob/index.js

Lines 154 to 164 in 0e07457

stream() {
const it = toIterator(this.#parts, true);
return new ReadableStream({
type: 'bytes',
async pull(ctrl) {
const chunk = await it.next();
chunk.done ? ctrl.close() : ctrl.enqueue(chunk.value);
}
})
}

zdm commented

Yes, or export the function and call it in the stream() method.
Let's do this, because this warning is really annoying. ;-)

jimmywarting commented

(It is also possible to run NodeJS with the flag --no-warning)

This project is still quite Browser & Deno friendly and dose not really depend on any built in NodeJS stuff and I would also like to keep it that way.
Deno still lacks ways of creating Blob/Files backed up by the FileSystem. So i'm having thoughts of using this in Deno for another project i have.

So perhaps stream.cjs should expose a global function instead: globalThis._loadStreams() (b/c .cjs can't use esm export)
I have been thinking of using conditional import with top level await, but that requires node v14.14...

zdm commented

Disable warnings is a bad practice, warnings are useful, but not in our case.
Are you really using deno? I am unable to imagine, where it can be used and what for. Only as hobby. ;-)

jimmywarting commented

Are you really using deno? I am unable to imagine, where it can be used and what for. Only as hobby. ;-)

Yea, I use Deno for some own hoppy stuff. not so much for work.
tried convincing them to use Deno, but that would be a major rework. If i start with a new project/startup then i would try to use Deno instead if possible.

zdm commented

I think the correct way is to export from streams.cjs function like this:

/* c8 ignore start */
// 64 KiB (same size chrome slice theirs blob into Uint8array's)
const POOL_SIZE = 65536;

var ReadableStream;

module.exports = function getReadableStream () {
    if ( !ReadableStream ) {
        try {
            ReadableStream = require( "stream/web" ).ReadableStream;
        }
        catch ( error ) {

            // TODO: Remove when only supporting node >= 16.5.0
            ReadableStream = require( "web-streams-polyfill/dist/ponyfill.es2018.js" ).ReadableStream;
        }

        try {
            const { Blob } = require( "buffer" );

            if ( Blob && !Blob.prototype.stream ) {
                Blob.prototype.stream = function name ( params ) {
                    let position = 0;
                    const blob = this;

                    return new ReadableStream( {
                        "type": "bytes",
                        async pull ( ctrl ) {
                            const chunk = blob.slice( position, Math.min( blob.size, position + POOL_SIZE ) );
                            const buffer = await chunk.arrayBuffer();
                            position += buffer.byteLength;
                            ctrl.enqueue( new Uint8Array( buffer ) );

                            if ( position === blob.size ) {
                                ctrl.close();
                            }
                        },
                    } );
                };
            }
        }
        catch ( error ) {}
    }

    return ReadableStream;
};

And then use it where you need ReadableStream.
And you will not pollute globalThis.

zdm commented

What do you think?
If you want I can send pull req.

zdm commented

Hey, man, are you alive? Sorry for disturbing you, but let's close this issue and forget about it.

jimmywarting commented

Sry. I have been busy on other projects

Uhm, okey. Why?

zdm commented

I mean not just close, I can send PR with the fix, if you want.
Pls, look at my comment above.

jimmywarting commented

one smaller change could also be to do:

const orig = process.emitWarning
process.emitWarning = () => {}
Object.assign(globalThis, require('node:stream/web'))
process.emitWarning = orig
zdm commented
alasks332 commented

This is a bad idea to publish stable release which is depends on unstable or beta apis.

I think, that web/stream should be removed from the stable release and moved to the next major pre-release version.
If somebody requires blobs with the streams support he can install pre-release version separately.

Warning, that produced by this module requires attention, and must be removed.

  • 👍28
Arondight commented

hi, will next version has a fix to this problem?

jimmywarting commented

I think, that web/stream should be removed...

with some degree i would be okey with that but i also need a prediction on what is best for the overall eco-system and guess what most ppl will use the most.

I advocate that ppl will prefer the latest version of web/stream first and formost, just b/c you don't need any polyfills and everyone will be using the same instance of stream 👈 most important

using the native streams by node also have some perf adv over a polyfill in a way that it's faster and can better be converted to from node/whatwg streams + that they are transferable via workers

The problem with web-streams-polyfill and any (native or polyfilled) ReadableStream is that they are not interchangeable with each other, this have been reported a numbers of times, and i have had problem with it myself in streamsaver.js and some other other streaming zip lib

to summarize all the above problem with a code example you can't do this kind of things:

import {WritableStream} from 'web-streams-polyfill'
new window.Blob().stream().pipeTo(new WritableStream({ ... })) // throws
new window.Blob().stream().pipeTo(new window.WritableStream({ ... })) // works okey

this is going to throw an error: TypeError: Failed to execute 'pipeTo' on 'ReadableStream': parameter 1 is not of type 'WritableStream'

this will also be a problem if you use two different web-streams-polyfill versions
i depend on web-stream-polyfill v3.0.1 and you have v3.0.2 then we can't interchange streams

import {ReadableStream} from 'web-streams-polyfill@3.0.1'
import {WritableStream} from 'web-streams-polyfill@3.0.2'

new ReadableStream().pipeTo(new WritableStream({}))

this is going to throw the same kind of error: TypeError: Failed to execute 'pipeTo' on 'ReadableStream': parameter 1 is not of type 'WritableStream' cuz they are not the same instance of streams.

this is the hole underlying problem of why i do not want to bundle node-fetch into a 0 dependency cjs module. cuz we would then have one version and you would be using another.

I would be comfortable with only using web-streams-polyfill if this got fixed

zdm commented

Initially node-fetch created as backend implementation of web api.
It should stay the same.
Blobs with the streams support - is non-standard object, and, as we can see, it brings more problems, then advantages.
Package should stay simple, be 100% compatible with the web fetch, and work without polyfils and other shit.
If somebody need stream - he can use .body.

  • 👍1
  • 👎1
jimmywarting commented

and other shit.

Language please!

Blobs with the streams support - is non-standard object

What do you mean by that? stream exist on blob.prototype.stream

Package should stay simple, be 100% compatible with the web fetch

That is what we are doing. staying true to what the spec is all about. Blob is part of fetch. to implement fetch you also need the other components such as Blob, File, FormData AbortSignal, EventTarget etc

it can't be simple and be 100% spec compatible at the same time. if we sacrifice a spec comp stream then it's not 100% spec comp. and some WPT-test would start failing

If somebody need stream - he can use .body.

involving node-fetch should not have anything to do fetch-blob and this issue b/c it's irrelevant.
try to see it as two separate packages. using res.body is not an option
eventually node-fetch will have whatwg streams as well and it's going to have basically the same thing as fetch-blob and using web/stream as well