Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

require "stream/web" produces warning #114

Closed
zdm opened this issue Aug 31, 2021 · 21 comments · Fixed by #125
Closed

require "stream/web" produces warning #114

zdm opened this issue Aug 31, 2021 · 21 comments · Fixed by #125

Comments

@zdm
Copy link

zdm commented Aug 31, 2021

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
Copy link

regseb commented Sep 1, 2021

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 zdm changed the title tream/web produces warning require "stream/web" produces warning Sep 2, 2021
@zdm
Copy link
Author

zdm commented Sep 2, 2021

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

@jimmywarting
Copy link
Contributor

jimmywarting commented Sep 4, 2021

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
Copy link
Author

zdm commented Sep 8, 2021

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 ) {}

@jimmywarting
Copy link
Contributor

jimmywarting commented Sep 8, 2021

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
Copy link
Author

zdm commented Sep 8, 2021

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

@jimmywarting
Copy link
Contributor

(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
Copy link
Author

zdm commented Sep 8, 2021

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
Copy link
Contributor

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
Copy link
Author

zdm commented Sep 10, 2021

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
Copy link
Author

zdm commented Sep 10, 2021

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

@zdm
Copy link
Author

zdm commented Sep 15, 2021

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

@jimmywarting
Copy link
Contributor

Sry. I have been busy on other projects

Uhm, okey. Why?

@zdm
Copy link
Author

zdm commented Sep 15, 2021

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

@jimmywarting
Copy link
Contributor

jimmywarting commented Sep 17, 2021

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
Copy link
Author

zdm commented Sep 17, 2021 via email

@alasks332
Copy link

alasks332 commented Sep 21, 2021

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.

@Arondight
Copy link

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

@jimmywarting
Copy link
Contributor

jimmywarting commented Oct 28, 2021

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

This comment has been minimized.

@jimmywarting

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants