-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: file sharing #76
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Thanks for opening this PR, this awesome. It seems like the frontend is failing to build, could you resolve those? If you intend to add support for the go-peer as well you may want to convert this to a draft PR. |
Thank you. I wanted to see CI logs but it seems I cannot because I don’t have an access to Vercel. Could you share mr which error occurred? For Go, I’ve seen that Go peer successfully send a request to the JS peer and receive a response, but the response body was empty. I’ll convert this PR into draft and will investigate it. |
Seems there was a compilation failure. npm ERR! Lifecycle script `build` failed with error:
--
07:32:50.873 | npm ERR! Error: command failed
07:32:50.873 | npm ERR! in workspace: universal-connectivity-browser
07:32:50.873 | npm ERR! at location: /vercel/path0/packages/frontend
07:32:50.890 | Error: Command "npm run build" exited with 1
|
@maschad I just fixed the compilation failure: 80f0963. |
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.
Awesome work on this! There are just a few comments.
Also I am going to ask for @TheDiscordian review for the Go aspect and @mxinden or @thomaseizinger review on this for the rust aspect.
Thank you for the awesome contribution @youngjoon-lee Maintenance - are you ok with being on the hook to maintain the file transfer aspects of this app? In case a user reports an issue here, is it ok for myself or others on the team to tag you/assign issues in that domain? Docs - I have a draft PR to add docs for this app here: libp2p/docs#331. It talks about libp2p only right now and needs to account for the chat UI etc. Would you be willing to contribute some docs on how the file transfer aspect works? i.e. For a new user, the goal is that we have docs accompanying the code so that they can follow to re-create this from scratch while understanding how things work. Note: I am happy to work with you to write these docs. How feature rich or sparse this app should be - This is an open question to all (pinging @2color for his 2 cents as well). This chat app was written to showcase how libp2p transports and implementations interop with each other. Adding file sharing capabilities seems like the natural next step.
or
|
3e78929
to
bb2cc94
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.
Rust code LGTM!
Sorry we have taken so long to address this @youngjoon-lee I think it's good for merge given you've addressed @TheDiscordian 's comments. Would you be able to resolve the conflicts and then we can land this? |
Given that I caused the conflicts with my other PRs, I went ahead and resolved them for you @youngjoon-lee. Couldn't push to this PR so opened one on your fork: youngjoon-lee#9 |
* Removed 'node' reference from main readme, trimmed down go-peer README to be more relevant to us (libp2p#75) * chore: Update .github/workflows/stale.yml [skip ci] * Add dependabot file (libp2p#86) * Move `frontend` to `js-peer` for consistency (libp2p#84) Co-authored-by: chad <[email protected]> * Update Rust peer to `0.52` (libp2p#83) --------- Co-authored-by: TheDiscordian <[email protected]> Co-authored-by: GitHub <[email protected]> Co-authored-by: chad <[email protected]>
Huh, that diff seems odd! Something about my merging must have gone wrong, sorry about that @youngjoon-lee :( |
No worries! Let me fix it :) |
My guess would be that just merging again should fix it! |
Thank you! @maschad. I resolved conflicts, with @thomaseizinger's help, and checked everything works well. Could you merge this PR if this looks good to you? |
Awesome! Thanks again @youngjoon-lee for your work here. |
Thank you. I'll keep maintaining this code by monitoring this repo. Please ping me whenever you want. |
Closes #69
Implemented the file sharing feature using streaming protocol, as presented as my HackFS 2023 project.
In Rust, I used the request-response protocol. I've noticed that the latest rust-libp2p v0.52.0 support request-response with CBOR/JSON codec, but I didn't use it in this PR. At later PRs, we can bump rust-libp2p.
In Javascript, I used the raw streaming protocol since js-libp2p doesn't have request-repsonse protocol explicitly.
Since both rust-peer and js-peer support WebRTC, two js-peer in browser can share files between each other.
TODO: Add file-sharing for go-peerI also implemented this feature for Go-peer.