feat: streams in browser

[AuHau]

I present you patch that allows usage of downloadDirectory and downloadObservable in browsers.

I was fighting a lot with readable-streams in browsers as there was some problem of proto-chain in Duplex streams. It seems that rollup is to blame for bad bundling. I switched it to webpack and it works as expected. Is that a problem?

I switched it only in the swarm-browser bundle and kept rollup in the timeline build. Should I unify it to webpack? From bundle size there was around 10kb increase in the production build to 242kB.

[AuHau]

Since there is downloadObservable as part of general API, maybe there could be downloadReadableStream() which would expose readable-stream's Readable interface?

Btw. not sure why the second build of Travis is failing. Local tests are working for me. Do you might have some pointers about what is wrong?

[PaulLeCam]

Thanks for your PR!

I restarted the macOS build but it's taking forever, might be an issue with Travis. It passes on Linux anyways so I don't think it's caused by your changes.

Do you have more details about the issue using rollup please? Is this a known bug?
If it can't be worked around I think it's better to use webpack for the timeline package as well but I'd rather avoid changing tooling to accommodate individual features.

Regarding adding downloadReadableStream(), what would be the use case please?

[AuHau]

Great!

Well there are several pointers for discussion about rollup and readable-streams. I wish I googled for them earlier than now 😂

• Stream is not defined in inherits call. nodejs/readable-stream#237 - mainly after point the issue was closed --> recommended using rollup-plugin-node-builtins yet that is now super outdated and does not work as well (I have tried)
• Circular dependencies (prevents proper rollup) nodejs/readable-stream#348 might be related as well
• Uncaught TypeError: Cannot read property 'prototype' of undefined calvinmetcalf/rollup-plugin-node-builtins#31

Also regarding the uploadFileStream(), in the end I did not move it to the browser implementation because there is no native support in browsers for WritableStreams and requests. It can be emulated like for example this library does: https://github.com/hugomrdias/iso-stream-http/blob/master/lib/request.js

But it only will bring compability with streams, yet it won't bring their actual advantage. Should I add it anyhow in order to unify the browser/Node API more?

Regarding the downloadReadableStream the use-case is mainly being able to utilize stream support of other libraries and the whole (nodejs) ecosystem. I know that Observable is in concept the same, but it does not have such support of libraries...

[PaulLeCam]

Thanks for the insights, the rollup/readable-streams seems like a real can of worms so better switch to webpack as you did. Could you also switch to webpack in the timeline package please so it's more consistent?

Regarding uploadFileStream() makes sense, can you remove the change in docs/api-bzz.md then please?

Adding downloadReadableStream() sounds like a good idea, maybe it could simply be named downloadStream() as it can only be readable anyways?
Could you please add tests to cover the behaviour both for raw files and manifests if you add this method?

Thanks!

[AuHau]

Will do! Most probably during tomorrow or on Monday.

…

On Thu, Nov 14, 2019, 15:18 Paul Le Cam ***@***.***> wrote: Thanks for the insights, the rollup/readable-streams seems like a real can of worms so better switch to webpack as you did. Could you also switch to webpack in the timeline package please so it's more consistent? Regarding uploadFileStream() makes sense, can you remove the change in docs/api-bzz.md then please? Adding downloadReadableStream() sounds like a good idea, maybe it could simply be named downloadStream() as it can only be readable anyways? Could you please add tests to cover the behaviour both for raw files and manifests if you add this method? Thanks! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#116>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABOKPOVZIOD2KUKVKW4X2JLQTVMZXANCNFSM4JM74RXQ> .

[PaulLeCam]

Thanks!

[AuHau]

So here I present you another version :-D

I added the stream support as discussed. Tests are passing, webpack integrated, docs written.

Let me know your thoughts!

[PaulLeCam]

@vujevits if you have the chance could you please review this PR?
I'm mostly wondering if this could affect the React-Native variant in a bad way? Does RN support node streams?

[vmaark]

@PaulLeCam seems like readable-streams had a related an issue and it's closed, so I suppose it should work. nodejs/readable-stream#319

[PaulLeCam]

@vujevits OK perfect, thanks for the feedback!

[AuHau]

So here is hopefully last version :-)

[PaulLeCam]

@AuHau CI is failing because of ESLint issues, could you run yarn lint:fix to address them please?

[AuHau]

Fixed (MacOS still runs as always)

[PaulLeCam]

Thanks for all the changes!