feat: module collection #28
Conversation
src/modules/collection.ts
Outdated
| for (const entry of data) { | ||
| if (entry.data instanceof Uint8Array) { | ||
| tar.addUint8Array(entry.path, entry.data) | ||
| } else if (isReadable(entry.data)) { | ||
| if (entry.size === undefined) { | ||
| throw new BeeError(`Size has to be specified for Readable data! [${entry.path}]`) | ||
| } | ||
|
|
||
| tar.addStream(entry.path, entry.size, entry.data) | ||
| } | ||
| } |
There was a problem hiding this comment.
It seems ineffective for performance, because it continuously checks its type of the data, when it could be supposed from the first element of the data object
There was a problem hiding this comment.
I wouldn't worry about performance here, because generally creating the tar and uploading it is orders of magnitude longer time than the check itself. Besides the code is now very straightforward and I don't really see how a change what you proposed could be implemented without casts.
There was a problem hiding this comment.
I think the casting is okay if we can save plenty of unnecessary checkings in loops. You're right about this snippet in the whole process is not the most resource consumable task, but we should strive for the optimal running time at every function regardless the whole context.
There was a problem hiding this comment.
I don't necessary agree to work on microoptimizations like this without measuring actual performance and seeing that it causes issues. Optimizations in general lead to less maintainable code.
There was a problem hiding this comment.
In my opinion, this is not micro-optimization and also can be handled in a nice way.
| */ | ||
| export async function upload( | ||
| url: string, | ||
| data: Collection<Uint8Array | Readable>, |
There was a problem hiding this comment.
NOTE: For browsers I would love to have support for FormData and use Multipart upload. Not within a scope for this PR
There was a problem hiding this comment.
I'm curious in which case you would prefer it over the FileList API (that we support).
There was a problem hiding this comment.
NOTE: For browsers I would love to have support for FormData and use Multipart upload. Not within a scope for this PR
Hi, does Bee now support this? I'm struggling with missing Content-type errors
We discussed that we postpone this optimization for now
This PR introduces the collection functionality, e.g. uploading multiple files and then accessing files from the collection.
I plan to not fix the issues with the
Readableinterface because I discovered that we may need to choose a different approach for the browser and it will most likely affect that as well. The issues with the browser are the following:tar-streamfor creating the tar in a streaming waybufferwhich is not polyfilled from version 5 of webpack (that is the version we use)readable-streamwhich relies onstreamwhich is also not polyfilledI also found that the generated JS bundle for the browser is already quite big.
Based on our previous conversations I would suggest a different approach, to try to use browser APIs and polyfill them on Node.js. On the server side the size doesn't matter that much anyhow. For example using something like this:
https://github.com/MattiasBuelens/web-streams-polyfill
This would require more research and maybe changing some packages that we depend on now but I would keep it as a separate issue and not as part of this PR.