Progress events without breaking pub-sub

[Gozala]

This is second take on #52, which end up breaking pubsub. It is based on #57 which fixed another issue.

With this changes fetch will use XHR if either options.onDownloadProgress or options.onUploadProgress is provided. Otherwise it will stick to native fetch. This way file.add could opt-in into observing progress, but have all of response be buffered before ability to consume it. Pubsub on the other hand doesn't really care about progress events and therefor will continue using fetch that can have a streaming response body.

[Gozala]

Electron seems to be failing because we use native fetch there, while we're passing non native AbortController. Prior to #54 we used node-fetch in electron so this issue did not came up. Then #54 just switched to XHR (using native APIs in Electron). And now that we're going for hybrid approach it became a problem, because native fetch in electron expects native AbortSignal instead of custom implementation from abort-controller

I have added abort-controller module, which prefers native implementation when available and falls back to the abort-controller library. Unfortunately we would need to upgrade all our uses of abort-controller (or at least of http-client) to swap those out with ipfs-utils/src/abort-controller. If choose to go this route I'll write a pull request to do it.

[Gozala]

Now this got complicated. It looks like http module makes use of any-singnal which in turn uses abort-controller. So even if native AbortSignal is passed it will be turned into non native. Switching any-signal to use AbortController from ipfs-utils would have been an option, but that would introduce circular dependency 😖

Not sure what's the best thing to do here is. I have created mysticatea/abort-controller#24

[hugomrdias]

I would really like to avoid #52 #54 #57 and this one by doing nothing or at best implement download progress (which can be done using fetch).
This is getting too complex and will only increase maintenance #54 is already and example of that.

Upload progress is and always was a dream thats why browsers never implemented it (or streams) any box between client and origin server can buffer the request and the progress means nothing. Hopefully in the future this will change (chrome is already implementing streams but warns about this problem).

Sorry for not supporting this, lots of good work went into making this, but i really think its best not to increase complexity for the upload progress.

This is just my opinion if others support this i will not block it.

[Gozala]

Upload progress is and always was a dream thats why browsers never implemented it (or streams) any box between client and origin server can buffer the request and the progress means nothing. Hopefully in the future this will change (chrome is already implementing streams but warns about this problem).

The main motivation here is to support upload progress in webui. Adding a large file takes a long enough that webui appears broken. E.g. 2gig file takes 10-12secs (for more details and numbers see ipfs/ipfs-webui#1534)

With this changes it was possible to render fairly accurate progrssbar. Your comment about box between server and client is correct, but it does not apply to the problem we were trying to solve, because webui talks to the local node.

I personally do not think that waiting on chrome to ship request streams, then waiting for it to propagate to the electron and giving up on non-chromium browsers in order to reduced complexity is the right tradeoff, because it leaves wrong impression of the IPFS ecosystem when someone is just trying to add a file.

However there might be alternative ways we could consider to provide a feedback, e.g. infinite spinners. They do help a bit, but don't really work for long tasks that take 10secs.

[jacobheun]

I think the UX improvement this adds outweighs the additional complexity. We should add some additional tests here to catch regressions. @Gozala can we add some tests to validate this actually fixes the streaming issue we had with pubsub?

While I did run this branch against js-ipfs previously and the tests passed, we shouldn't need to rely on that suite to verify things are working as expected.

[lidel]

I personally do not think that waiting on chrome to ship request streams, then waiting for it to propagate to the electron and giving up on non-chromium browsers in order to reduced complexity is the right tradeoff, because it leaves wrong impression of the IPFS ecosystem when someone is just trying to add a file.

I agree with @Gozala on this, shifting burden from the library to its user feels like a bad trade off here.

We document progress handler at js-ipfs/docs/core-api/ and it is a terrible devexp when it really does not work over HTTP API.

:+1 on adding tests for:

• pubsub fix
• progress reporting: namely, modify existing test to expect at least one intermittent event between 0% and 100% (to guard against silent regressions where only final "finished" event is emitted.

[achingbrain]

This is the wrong solution as it ignores what's actually going during the API call (e.g. the HTTP request and the API request may have different notions of 'progress'), but sadly the right solution is completely impractical right now (bidirectional HTTP streaming in browsers) so it'll have to do.

Can you also please:

1. Open a PR against js-ipfs with this branch as the ipfs-utils dep version to make sure the tests pass with these changes
2. Expose and document the new upload/download progress args in ipfs-http-client
   • These probably just need documenting under additional options in the ipfs-http-client README, not the core-api docs as they make no sense in the context of calling into core/running a full IPFS node in the browser or node.js
3. Ensure it doesn't cause a regression for Ipfs-http-client throws an error when running jest/jsdom unit tests js-ipfs#3238

[Gozala]

1. Open a PR against js-ipfs with this branch as the ipfs-utils dep version to make sure the tests pass with these changes

ipfs/js-ipfs#3310

2. Expose and document the new upload/download progress args in ipfs-http-client

   • These probably just need documenting under additional options in the ipfs-http-client README, not the core-api docs as they make no sense in the context of calling into core/running a full IPFS node in the browser or node.js

I added jsdoc comment but have not added it the README, because I don't think we want users to pass this option as it would disable streaming. It is only used by ipfs.add and even there user does not need to provide additional option, existing progress handler is used.

3. Ensure it doesn't cause a regression for Ipfs-http-client throws an error when running jest/jsdom unit tests js-ipfs#3238

Verified

[Gozala]

Had to reset on top of current master, otherwise github complained about non-clean merge even though I did merge master in 🤷‍♂️

[Gozala]

merged #62 into this as there was some overlap between two

[achingbrain]

Can you fix the conflicts please? Then we should try to get this resolved.

[Gozala]

So #62 has introduced @achingbrain/electron-fetch as a dependency, which is API incompatible with both node-fetch and native fetch 😭, more specifically native and a node-fetch Response implementations accept Uint8Arrays for a body, but electron-fetch does not:

https://github.com/achingbrain/electron-fetch/blob/b6a8cb74b4f04837140044af3396e150a5ee2e8c/src/body.js#L24-L44

Which causes them to be treated as strings leading to incorrect behavior here:

js-ipfs-utils/src/http/fetch.node.js

Lines 82 to 99 in 8111a80

	/** @type {Buffer|null|Readable} */
	if (body == null) {
	onUploadProgress({ total: 0, loaded: 0, lengthComputable: true })
	} else if (Buffer.isBuffer(body)) {
	const total = body.byteLength
	const lengthComputable = true
	yield body
	onUploadProgress({ total, loaded: total, lengthComputable })
	} else {
	const total = 0
	const lengthComputable = false
	let loaded = 0
	for await (const chunk of body) {
	loaded += chunk.byteLength
	yield chunk
	onUploadProgress({ total, loaded, lengthComputable })
	}
	}

Furthermore unlike node-fetch electron-fetch represents response.body in different representations from node-fetch, which creates more problems
https://github.com/node-fetch/node-fetch/blob/cbd4d895767e33491219e40013dd8daa5c7ac024/src/body.js#L35-L69

What's worth both seem to ignore that response.body supposed be ReadableStream.

[Gozala]

@achingbrain I've update patch so that it overcomes issues mentioned in last comment. While it would make sense to make changes to electron-fetch to make it more API compatible with node-fetch, I would really really like to avoid blocking this further, and pursue that separately. This is already several degrees separated from the actual task of having a progress bar when you import files in webui.

[lidel]

As long, we have tests that guard against regressions (see comment inline), lgtm!

@achingbrain we need this to unblock improved import UX scheduled to land as part of pinning service integration:

• File(s) import progress ipfs-webui#1594
• feat: upload progress ui ipfs-webui#1655

[achingbrain]

Is this really necessary? We don't use jest and this file should only be used in the browser which wouldn't be using node-fetch.

[Gozala]

You told me in the earlier comment to:

Ensure it doesn't cause a regression for ipfs/js-ipfs#3238

Which is why this done here.

[Gozala]

@achingbrain can you please let me know what else needs to be done here ?

[achingbrain]

@hugomrdias could you eyeball these changes and release if you're happy with them please?

[hugomrdias]

Sure I will do it asap. Hugo Dias

…

On Fri, Nov 13, 2020, 13:57 Alex Potsides ***@***.***> wrote: @hugomrdias <https://github.com/hugomrdias> could you eyeball these changes and release if you're happy with them please? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#60 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACMWTXHSN5J7WYNK2LN6GDSPU3OFANCNFSM4QCOMJPA> .