-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
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 demo available here is not an interop
demo, in fact, it doesn't touch go-ipfs at all, I know you wanted first to do js-ipfs to js-ipfs, but that is not the purpose.
The demo must be (as discussed during prep session):
- User adds content using go-ipfs CLI
- User accesses that content from a browser.
To overcome the challenge of not being blocked by the stream muxing issue, you can write the whole tutorial using js-ipfs CLI + js-ipfs in the Browser, then when muxed is fixed (cc @victorbjelkholm), we will just need to s/jsipfs/ipfs
.
examples/interop-examples/README.md
Outdated
@@ -0,0 +1,102 @@ | |||
# interop examples |
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.
s/interop examples/fetch content added by go-ipfs
examples/interop-examples/README.md
Outdated
npm start | ||
``` | ||
|
||
Open http://127.0.0.1:8080 in your favorite browser. |
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.
Chrome or Firefox
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.
Both should work, thus "your favorite" :)
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 is also Safari, Edge, Internet Explorer and Opera, not everyone's favourite is Chrome or Firefox :)
examples/interop-examples/README.md
Outdated
Steps | ||
1. create IPFS instance | ||
|
||
First, we need an IPFS instance that we'll use throughout the tutorial. For the sake of convinience, we'll use `ipfs-daemon` module to create and IPFS instance. |
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.
We should not be recommending ipfs-daemon
in order to instantiate js-ipfs
, it just means that js-ipfs
needs to improve its init process.
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 rework this in a way that the init dance is in its own file and we include js-ipfs
instead of ipfs-daemon
.
I don't think we'll get to merge ipfs-daemon to js-ipfs within two weeks given how massive public API change that is. Let me know if you think we can.
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 think we'll get to merge ipfs-daemon to js-ipfs within two weeks given how massive public API change that is. Let me know if you think we can.
We totally can, it is not actually that big of a change, really, just requires putting in the hours, it is pretty clear what we want out of it and we have a ton of tests creating js-ipfs daemons with different configurations and in Node.js + Browser, those that will ensure that the new API works.
It is, however, a low priority compared to the rest of the tasks for this Sprint, so, if we get to finish all the others, we will want ot tackle this.
Until then, the demo should be without an external module to create an instance of this module.
examples/interop-examples/README.md
Outdated
}) | ||
``` | ||
|
||
2. listen for file drop |
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 goal of this demo is to "access files from go-ipfs", and not adding files inside the browser.
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.
Understood.
Somewhere we discussed, and decided, that we do go-ipfs -> js-ipfs-browser -> js-ipfs-browser (stream a video or smth). I believe adding the support for adding files to the browser version is not that much out of scope in terms of work and it adds another important step to the tutorial for one of the most asked questions for someone who's starting with ipfs "how do I add files to IPFS?". So having that functionality gives a more fully fledged tutorial than just catting files from go-ipfs. Don't worry, we're still going for the same goal: access files from go-ipfs.
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 :)
All of this is what the example will be. Perhaps the intent was dropped in the communication. If you take a look at my sprint updates, you'll see the progress being described (ie. it's WIP). See this comment #714 (comment) for details to why there's browser-to-browser functionality too. |
To clarify, this PR will (eventually) implement the example as designed here: #711 (comment) |
@haadcode thank you for clarifying! :) |
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.
@haadcode just tried the example and worked like a charm.
But I didn't really run through the Tutorial, because the example is the full blown thing already and I understand why, we need to have the thing done once, in order to know it works, so here is my suggestion to improve the UX.
create a folder calledtutorials
at the root of the project and move this example there- rename the folder to 'access-go-ipfs-files'
- inside the folder have
README.md, final/, start
- inside the folder
final
, have the full built thing - inside the folder
start
, have just the starting files (package.json, http server step up, etc) - in the README, explain the purpose of this tutorial, how people can run the real deal and that they can do this as a tutorial.
- Important to add to
gitignore
the files from thestart
folder, with the exceptions of the ones you leave there :)
Apply this to other example as well.
Note that, things that are still missing from the goal of this example (which you are probably well aware, but just listing them now to keep track):
- right now, it is still just doing browser to browser
- There is no reference as to explain that a file should be added with js-ipfs/go-ipfs CLI
- It needs to explain that a WebSockets address needs to added to go-ipfs/js-ipfs
- or a webrtc-star address to js-ipfs
examples/interop-examples/README.md
Outdated
|
||
## Run | ||
``` | ||
cd examples/interop-examples |
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.
no need, people will be inside this folder already
"author": "", | ||
"license": "ISC", | ||
"dependencies": { | ||
"ipfs-daemon": "^0.3.0-beta.16" |
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.
When ipfs-daemon
is out of the equation, it will be so much faster to get this example to work because one less npm install of the whole thing is needed.
"start": "http-server -c-1" | ||
}, | ||
"author": "", | ||
"license": "ISC", |
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.
s/ISC/MIT
@haadcode I was going to review this example again, but I see there hasn't been any other commit since my last review, so I will avoid listing out the points of feedback again. I received a notification through IRC that you were preparing some handoff guide for the next person to take on this examples, however, I'm being unable to find it, I've scanned the IRC logs and opened all the related github issues, could you point me to it? Thank you. |
Working on it. |
Updated everything, fixed a bunch of stuff. This should be now ready to be picked up by others. What remains to be done is listed in: https://github.com/ipfs/js-ipfs/blob/e916a5d1d0c654a6111e5a4861aafb79b72b3e62/examples/access-go-ipfs-files/cat-a-file/README.md#todo. There's also the new signal server address that needs to be fixed as per libp2p/js-libp2p-webrtc-star#87. The short is: add the UI for connecting to a go-ipfs address manually and make sure catting the file from go-ipfs-->js-ipfs(-browser) works. |
Seems that it is assumed that the user has ran
|
The example doesn't have a way to signal a want to dial to any other node, assumes that I rebased the commits |
e916a5d
to
ed49df6
Compare
working on this example today |
ed49df6
to
4c5443a
Compare
using only webrtc-star as a transport
af0ff7a
to
06aa615
Compare
Todo notes left by @haadcode for @RichardLitt
|
Taking over this for now |
Sorry @haadcode missed your comments before, reply inline:
I would like the layout to stay the same no matter the viewport so I'll go with px here.
Sure, no problem. I'm not gonna spend any time on trying it on mobile though, if people want it to work on mobile, they'll have to solve any issues themselves (or we can do another example for mobile in the future)
Not really, I've removed the picture since I didn't see how it tied into the example. I think it's not necessary to show a picture inline in the webpage but rather when people add files, we show them in a download list. |
c50e642
to
8ce1e98
Compare
Last update before weekendStill missing some refactor, mainly splitting up the ipfs and UI logic, right now it's all wrapped together. Made just one quick pass for comments so probably there are things that can be better explained. I have not looked at the tutorial and I'm pretty sure it's unfinished. Did some improvements with the drag-and-drop but still there is a bug left. Biggest one is that
|
- refactor: reorg + fix lint - fix: build step, no longer fail on postinstall - docs: add diagram - docs: add the instructions steps - lint: fix all the things - note about signalling server
- nicer colors - include logo - fancier buttons - easier flow to start daemon - less information after starting daemon - use dns signal server - larger font size - removal of unused features - removal of duplicated script references - more explanation - and probably something more I forgot
@victorbjelkholm I just fresh git cloned and fresh install both package.json and this is what I'm getting: Updated: fixed in #31c6383 |
d622347
to
31c6383
Compare
Final pass-through before leaving this move on to the CI/CD sprint
Conclusion
|
Discovered via @diasdavid doing a demo today with this.
|
@victorbjelkholm fixed the get problem. The stream was never resumed :) |
Follow here: #774 (comment) |
This PR will add a basic interop example as per #711.