Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

feat: move regular files api to top level, add addFromFs and addFromUrl #378

Merged
merged 10 commits into from
Nov 12, 2018

Conversation

daviddias
Copy link
Contributor

Related to #118

@ghost ghost assigned daviddias Oct 25, 2018
@ghost ghost added the in progress label Oct 25, 2018
SPEC/FILES.md Outdated
- [catPullStream](#filescatpullstream)
- [get](#filesget)
- [getReadableStream](#filesgetreadablestream)
- [getPullStream](#filesgetpullstream)
- [ls](#ls)
- [lsReadableStream](#lsreadablestream)
- [lsPullStream](#lspullstream)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving all the non MFS commands to the top level. This preps the users for the future Files API as they will get used to call the add, cat, get and ls from there, but when the MFS parts get coalesced with the non MFS parts, they will just get the lovely surprise that there will be a "drive" looking thing that contains all the files they added.

SPEC/FILES.md Outdated
- [addReadableStream](#addreadablestream)
- [addPullStream](#addpullstream)
- [addFromFs](#addFromFs)
- [addFromUrl](#addFromFs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the addFromFs (no one should have to do this: https://github.com/ipfs/js-ipfs/blob/master/src/cli/commands/files/add.js#L18-L102) and addFromUrl.

Once ipfsx is ready, we can squash all these 5 adds into one function.

@daviddias
Copy link
Contributor Author

Happy to work on these changes if everyone feels great (or ok) about them :)

@achingbrain
Copy link
Collaborator

Why not go all the way and make MFS top level? Considering most MFS commands also take /ipfs/QM.. as valid paths..

@daviddias
Copy link
Contributor Author

@achingbrain I believe that is the plan for the future with the whole API redesign. Unfortunately there are some conflicts such as .ls and if we don't address them at the internals level, it will be hard to pick what is correct.

@alanshaw
Copy link
Contributor

Yep, and IPFSX already has a unified ls:
ipfs-shipyard/ipfsx#1
https://github.com/alanshaw/ipfsx/blob/master/API.md#ls

@achingbrain
Copy link
Collaborator

I think we should jump straight to the ideal API, rather than make users update their code twice - once to this and then again to ipfsx.

@daviddias
Copy link
Contributor Author

@achingbrain even if that means 4+ months?

In order for ipfsx to land, we will need:

  • (A) Refactor the whole codebase plus test that a stream muxer powered by async generators will be equally performant as the one we have with Streams.
  • (B) Agree on the de-facto files API

I believe that we should do these in parallel

@alanshaw
Copy link
Contributor

I'm +1 on moving the files API stuff to the top level. IPFS's prime purpose is to deal with files, so these calls should be privileged. They are also the most widely used calls so saving on typing helps developers. WIN WIN.

I think that addFromURL and addFromFs are convenient right now, but these operations will become really easy if add starts to take an async iterator like IPFSX does and so I don't think they'll be needed.

Right now in IPFSX if I want to add these things I'd do something like:

// Add a file
const stream = fs.createReadStream('/path/to/file')
const { cid } = await node.add(stream).first()

// Add a URL
const response = await fetch('http://giffatron.com/cat.gif')
const { cid } = await node.add(response.body.getReader()).first()

Ok so, caveat, browser ReadableStream isn't a async iterator quite yet but we can shim temporarily.

@daviddias
Copy link
Contributor Author

@alanshaw what about users that want to add entire directories from their local file system in Node.js (analogous to the CLI ipfs add)? Even with ipfsx, you would still need to do the globbing manually that is happening here https://github.com/ipfs/js-ipfs/blob/master/src/cli/commands/files/add.js#L18-L102 every single time a user is using a embedded node.

@hugomrdias
Copy link
Contributor

I agree with @achingbrain we should not do a breaking change just to do another in a few months , I don't believe there sufficient value in it. We can improve docs and make small tweaks to improve experience though.
I think we should aim for ipfsx I looked into it just now for a second time didn't know it was already so complete. Looks freaking awesome @alanshaw please tell me how I can help !! We need this 🎉🎉🎉

@alanshaw
Copy link
Contributor

@alanshaw what about users that want to add entire directories from their local file system in Node.js (analogous to the CLI ipfs add)?

Good point, I didn't consider that.

@daviddias
Copy link
Contributor Author

@hugomrdias here is a good milestone towards that. Convert https://github.com/libp2p/js-libp2p-mplex to async-generators and run the stream muxer tests on it https://github.com/libp2p/interface-stream-muxer/tree/master/src and compare it with the current implementation in terms of speed and memory usage.

@daviddias
Copy link
Contributor Author

daviddias commented Oct 25, 2018

Can I get a quick vote? I want to make sure to understand if people agree/disagree if people want to ship A and B -- #378 (comment) -- in one swipe.

@alanshaw
Copy link
Contributor

I think we should aim for ipfsx I looked into it just now for a second time didn't know it was already so complete. Looks freaking awesome @alanshaw please tell me how I can help !! We need this 🎉🎉🎉

❤️

There's so much to do. I'm trying to create RFC issues for everything now so that there can be some discussion before an implementation lands. The biggest contribution you can make is using it, suggesting changes, and proposing things you'd like to see.

Also, is there any opposition to moving the ipfsx repo to ipfs shipyard or is it too soon? I think by moving it to shipyard the community might feel it has some longevity and be more open to try it out and it'll also aid discovery.

@daviddias
Copy link
Contributor Author

is there any opposition to moving the ipfsx repo to ipfs shipyard or is it too soon? I think by moving it to shipyard the community might feel it has some longevity and be more open to try it out and it'll also aid discovery.

Yeah, do it :)

@daviddias
Copy link
Contributor Author

From IRC (important for context on the vote #378 (comment)):

image

SPEC/FILES.md Outdated
- [catPullStream](#filescatpullstream)
- [get](#filesget)
- [getReadableStream](#filesgetreadablestream)
- [getPullStream](#filesgetpullstream)
Copy link
Contributor

Choose a reason for hiding this comment

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

#filescatreadablestream => #catreadablestream...and others ;)

@daviddias
Copy link
Contributor Author

Have it all working on ipfs-inactive/js-ipfs-http-client#878. Now onto js-ipfs.

@alanshaw @hugomrdias @achingbrain Please review meanwhile the docs and tests in this PR. Thank you!

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@ghost ghost assigned alanshaw Nov 12, 2018
Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

I've added some typo fixes. I'm assuming the tests for addFromFs, addFromUrl, addFromStream have been copied from js-ipfs-api so I haven't reviewed them in depth (since they should have already been reviewed!).

Alan Shaw added 3 commits November 12, 2018 13:40
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@alanshaw alanshaw merged commit 3dc7278 into master Nov 12, 2018
@ghost ghost removed the in progress label Nov 12, 2018
@alanshaw alanshaw deleted the files-api-tweaks branch November 12, 2018 16:35
lidel added a commit to ipfs/ipfs-companion that referenced this pull request Jan 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants