Conversation
lidel
left a comment
lidel
left a comment
There was a problem hiding this comment.
Exciting! Some quick feedback:
- I feel we should fix
files.writebefore this is merged to avoid issues described in #676 - In files listing, icons should be clickable and have
pointercursor (right now only file name is) - Switch to streams is optional (If API is backed by
js-ipfs-apiit buffers entire thing anyway, I think).
| promises.push(readAsBuffer(file)) | ||
| } | ||
| for (const file of files) { | ||
| promises.push(readAsBuffer(file)) |
There was a problem hiding this comment.
This may be a separate PR, but we may want to replace readAsBuffer with pull stream to improve performance when adding big files:
There was a problem hiding this comment.
Yes please. Is ok for separate PR, but we've gotta make this the best file uploader we can.
| return Promise.all(files.map((file) => { | ||
| await Promise.all(files.map(file => { | ||
| const target = join(root, file.name) | ||
| return getIpfs().files.write(target, file.content, { create: true }) |
There was a problem hiding this comment.
We should avoid files.write, as it will produce different CID than add.
See details and workaround in: #676
hacdias
changed the title
hacdias
force-pushed
the
feat/revamp/upload-options
branch
from
7579896 to
fe048c4
Compare
|
@lidel I just made the files.write change and gave the dropdown some styling. I'll add each feature in a different PR to keep everything small. Could you see if the code's looking good? |
lidel
left a comment
lidel
left a comment
There was a problem hiding this comment.
- After upload is finished menu with "Add" actions should close
- Missing some kind of visual feedback and Adding bigger files. Perhaps putting
%on label while upload is happening would do for now? (similar to how we handled Download for now)
| const target = join(root, file.name) | ||
| return getIpfs().files.write(target, file.content, { create: true }) | ||
| await Promise.all(files.map(async file => { | ||
| const res = await getIpfs().add([file.content]) |
There was a problem hiding this comment.
I wonder if we should disable pinning when adding for MFS.
As long as files are in MFS, they won't be GC'd.
If we dont pass {pin: false} to add then removing file from MFS won't remove it from node during next gc, and this disk-space leakage may confuse less technical users.
Is that a bug or feature ?
@olizilla any thoughts on this?
There was a problem hiding this comment.
I'm adding that option then.
About the progress of uploading: don't worry. Won't be doing in this PR but it is on Invision how it is supposed to be.
|
Done @lidel |
| return `${a}/${b}` | ||
| } | ||
|
|
||
| export function dirname (path) { |
There was a problem hiding this comment.
Let's just use node core path stuff for this kind of thing, https://nodejs.org/api/path.html#path_path_dirname_path
There was a problem hiding this comment.
Of note this file was intended to be a lib for ipfs/ipld path helpers, rather than any sort of path.
olizilla
left a comment
olizilla
left a comment
There was a problem hiding this comment.
Is good work. Please use node path module for dirname and join stuff.
|
Done @olizilla |
|
if nothing else is using our |
3 participants
Uh oh!
There was an error while loading. Please reload this page.