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

refactor: simplify file adding #769

Merged
merged 3 commits into from
Aug 29, 2018
Merged

refactor: simplify file adding #769

merged 3 commits into from
Aug 29, 2018

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Aug 29, 2018

@lidel with your comment yesterday (#762 (comment)) I ended up changing the code and simplifying it a bit.

Although, it happened once or twice to upload like 10 streams and it would only return 1 hash which I think it might be related to https://github.com/ipfs/js-ipfs-api/issues/797!

It also happened before, but since it was only one by one, they wouldn't all fail.

License: MIT
Signed-off-by: Henrique Dias [email protected]

License: MIT
Signed-off-by: Henrique Dias <[email protected]>
@hacdias hacdias requested a review from lidel August 29, 2018 07:37
@hacdias
Copy link
Member Author

hacdias commented Aug 29, 2018

Fact is: the same set of files errors when using go-ipfs, but it always works on js-ipfs.

@hacdias

This comment has been minimized.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Tested:

  • adding multiple files (flat)
  • adding a directory with multiple levels of subdirs (350MB tree of mp3 and flac from Jamendo)

@lidel
Copy link
Member

lidel commented Aug 29, 2018

@hacdias

FYI IPFS Companion already applies a fix for ipfs/js-ipfs-api#797 with local node under Firefox, but Chrome does not allow us to touch Connection header so we should have additional error handling for Chrome users.

There should be an additional check to avoid silent failures (but it can be separate PR):

  1. count the number of added items
  2. count the number of results returned by files.add (+1 if wrapping dir)
  3. if counts do not match, do not copy results to MFS, but display error message
    • I dont think users care about details of ipfs/js-ipfs-api#797, so it should say something simple and actionable like "Unable to finish upload: IPFS API returned a partial response. Try adding a smaller tree." (should heal most failures, if user adds different set of bytes the bug will probably go away).

License: MIT
Signed-off-by: Henrique Dias <[email protected]>
License: MIT
Signed-off-by: Henrique Dias <[email protected]>
@hacdias
Copy link
Member Author

hacdias commented Aug 29, 2018

@lidel thanks! Just added it here!

@hacdias hacdias merged commit dfd0a36 into revamp Aug 29, 2018
@hacdias hacdias deleted the simplify-adding branch August 29, 2018 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants