-
Notifications
You must be signed in to change notification settings - Fork 1.2k
tutorial: File Exchange - Transfer files between desktop and browser using go-ipfs, js-ipfs and orbit-db #716
Conversation
This example is looking very similar to PaperHub that @dignifiedquire made a while ago, it is interesting, but it the main goal is to have a file manager that can open directories and traverse to them, these directories should be added through go-ipfs. I know you are still heavily working on this one and that you have the intent of making it the actual File Browser, so I will review it again when it is fulfilling that goal. |
I believe that this 'injection' of orbit into the example is creating a distraction to the main goal, which is why developing it incrementally, as suggested, would have been a big win. Now, the better is just consider the time-left/effort to achieve that main goal and see if we get closer by taking out orbit out of the equation or not. |
@haadcode updated the name of this PR to reflect the decision made through IRC chat. One task that needs to also happen is:
|
The browserify-zlib-next dependecy problems have been solved. What is still needed is to merge this: ipfs-shipyard/browserify-zlib-next#23. cc @diasdavid @dignifiedquire |
I'm fixing and updating everything right now to make sure this can be finished by someone. |
|
Thank you @dignifiedquire 🎉 |
Can verify that everything works now with browserify-zlib-next 1.0.1. Hooray \o/ |
3313cf3
to
07d97fe
Compare
TODOs left by @haadcode TODO
|
|
} | ||
this.ipfs = node | ||
this.emit('ready') | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Async operations in a constructor are an anti-pattern, a good source of errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah got it, this always happens if we open without being on a feed which is controlled by the top level domain. This is a bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this is not good. Will change it.
@haadcode how did you manage to use WebPack@2? I'm missing to find any note and ipfs doesn't really work if bundled with WebPack@1 and as @dignifiedquire just mentioned, WebPack@2 will only come in the next version of create-react-app. If there was alternative, shall we eject and update the react deps? |
074e1f1
to
8b826c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the App isn't in a state where it is easy to use it, as it shows several bugs during its usage. It might be necessary, since it is a complex app, to have tests for it, it will help the debugging while the remaining of the development is finished.
I've structured it in the same way the other tutorial is, added a bit more information to the Readme and talked with @RichardLitt about making the full tutorial. I'll still add a diagram of the app.
@haadcode I would appreciate your help on getting this ready for Monday, I'll be around Sunday when you come back for us to catch up :)
npm-debug.log | ||
|
||
# orbit-db | ||
orbit-db |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to ignore a folder named orbit-db
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OrbitDB writes its cache file to that directory when run with go-ipfs or js-ipfs from the CLI. This is per user and we shouldn't add it to the repo.
examples/file-feed/README.md
Outdated
## Step-by-step instructions | ||
|
||
`TODO` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RichardLitt is on track to do this once the example is completely done. @RichardLitt I'll notify you when that happens ❤️
net: 'empty', | ||
tls: 'empty' | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we validate that we actually need all of this configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we need all this configuration. See my long comment below as to why.
Short: the webpack config file here is the create-react-app/react-script config file with the only difference that we add the zlib shim to it.
@@ -0,0 +1,25 @@ | |||
{ | |||
"name": "file-feed", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed it to file-feed
as I'm afraid of stamping something with a "InterPlanetary X" name that is not a project (i.e example/tutorial) that we support and make it have the same level of activity of a product.
Pointing people to the 'file-feed tutorial' has a completely different tone than 'InterPlanetary FileExchange'
@haadcode hope you understand, we can chat about it and reach an agreement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diasdavid I'm totally fine changing the name! However, let's catch up on IRC today to discuss the name as I think we can find something more descriptive and less "loaded" word than "feed" (people will make assumption about what a fedd is that I think we can't provide in this example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be around for the rest of the day
{ | ||
"name": "file-feed", | ||
"version": "0.0.0", | ||
"bin": "src/ff-cli/bin.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also put the CLI under the bin path, for ease of use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good! I was thinking we could yargsify the CLI tool so that we can run
ff-cli daemon hello-world
and ff-cli add hello-world file.txt
instead of having two separate programs. This would make the code sharing between the two also a lot easier. Unless you're against yargsifying it, I'll go ahead and do that today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 not against at all :)
} | ||
|
||
getFile (hash) { | ||
return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Back in the January meetings, we talked about using Promises in examples and we agreed at the table that we would use only callbacks for our examples. Is the use of Promises strictly necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember us talking about Promises, but sure, we can make these callbacks too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
stream.on('end', () => { | ||
this.emit('load', hash, bytes) | ||
resolve(buffer) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use bl
or concat
for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer not adding a library to abstract the underlaying functionality of the stream. After all, this is an example and people will have different approaches to how they use streams, some will use bl
or concat
, some won't., so keeping it clear what ipfs returns is clearer in this case imo.
|
||
addFiles (file) { | ||
const addToIpfs = (name, content) => { | ||
return this.ipfs.files.add([{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IPFS might not exist in this context due to the async operation in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Will fix the constructor problem.
} | ||
|
||
const addToOrbitDB = (file, type) => { | ||
return this.feed.add({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.feed
doens't exist if we aren't in a path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, feed is locally scoped. Will address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was fixed in a7b6049. Constructor is not async anymore.
|
||
setInterval(checkForPeers, 1000) | ||
setInterval(checkForFiles, 1000) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The daemon
and the cli
tool share a ton of code, do we really need too files for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my previous comment re. yargisyfying and combining the two program into one. Yes, we should share the code :)
feature request: A very cool features would be to 'watch' a folder, and for every file to gets added to the folder, it syncs to the browser and for every file that is added to the browser, it syncs back to the folder. |
97be250
to
7d120c6
Compare
Could you squash now? If we get into the habit of pushing clean commit messages, it will be easier to keep it consistent |
Hey @pgte it seems that this example froze in time, however, it is pretty valuable. Could you take a look and make it work just with IPFS primitives? You would end up creating a 'small crdt-log' to make it work on top of pubsub, but if that is a single file vs a full external dependency, I feel that would create way more value and direction for the users. |
@diasdavid sure, I'll look at this once I'm done with ipfs-level. |
Closing this PR as the example was never developed further and since then there were a ton of advancements in js-ipfs and orbit |
This is a WIP PR for the second interop example. A File Manager in the browser, which eventually will be able to display files added in go-ipfs. It currently works only between browser js-ipfs nodes.
No need to review it yet, still working on it.