theobat commented 7 years ago

Simple

[x] use strict in *.config.js
[x] config/ipfs-daemon.config.js pass path instead of OrbitConfig
[x] Answer this :

Is there a reason the MODE is passed with the settings? Generally the MODE is to just distinguish between dev mode and production mode and imo shouldn't be part of settings per se (ie. not something user can set in the configuration screen).
[x] clean client/src/components/App.js Logger level
[x] solve instantiation problem with ipfs-daemon.config.js

Composed (refactoring IpfsDaemonConfig)

[x] onPersist/onRetrieve renaming
[x] create orbit instance on connect
[x] store orbit instance in IpfsDaemonStore
[x] clean network store : + onConnect: function(host, username, ipfsInstance) {

theobat commented 7 years ago

@haadcode, I am struggling with this error:

app.js:48 TypeError: ipfs.object.put is not a function(…)

Which comes from the fact that I pass an ipfs instance through ipc communication in electron (from backend to frontend once daemon started successfully) and this communication serialize objects as JSON (so functions disappear).

Do you have any idea how I could communicate full JS objects between main and renderers in electron ? I have not been able to find any solution to that yet

haadcode commented 7 years ago

@theobat Hmmm... afaik Electron supports full objects in the IPC, so that shouldn't be the problem. What you could try is to not pass the IPFS instance as a param of the event (mainWindow.webContents.send('ipfs-daemon-instance')), but save it into global.IpfsInstance (like it was) and on the client side when you receive the event, just read it from global.IpfsInstance.

theobat commented 7 years ago

Seems it's using JSON.stringify in the ipc functions: https://github.com/electron/electron/issues/3923

But the global variable might be a good solution, thanks !

haadcode commented 7 years ago

@theobat let me know when you want a code review on this PR.

Making good progress I see! :)

theobat commented 7 years ago

So this PR is almost ready, you can probably start the review but bear in mind the configuration page is ugly (because there is no styling at the moment).

A few remarkable things:

The ipfs daemon is started 'onConnect', with a default configuration for electron like it was before
The persistence of the ipfs config is triggered only on "return' statement (not on every change, they stay in the state)
The persistence tool is localStorage not a file like you suggested. I think it's better because it does not change the logic between electron and the browsers.

And a question for the mergability of this, should I build everything ?

haadcode commented 7 years ago

@theobat I'll start reviewing this now. First I'll check how it works, looks and feels and then move on to code. Would you prefer feedback line-by-line for the code or would you prefer me doing a PR against this PR and do changes directly in the code?

theobat commented 7 years ago

@haadcode as you wish, it probably depends on whether you feel there's a lot to change in the current PR or not... But I'm available today to make changes as you request them. Hold on just a sec for another push (although it's just styling it does make a difference for your eyes)

haadcode commented 7 years ago

I'm trying to run Orbit with this PR but get the following errors after hitting "Connect" in the login screen.

screen shot 2016-11-16 at 10 08 58

theobat commented 7 years ago

@haadcode, thanks for the thorough review ! gonna try to change all this today. I get it for the Orbit object and design. The default ipfs-daemon-configs are not passed properly it seems. First a push with ui stuff and then clarifying the whole process.

haadcode commented 7 years ago

Thanks @theobat! Looking forward to wrap this up and merge :)

haadcode commented 7 years ago

Leave a comment when this is ready for a new CR.

theobat commented 7 years ago

@haadcode, so overall this looks way better now and works pretty well. I still have a problem when shutting down the ipfs daemon though, it is called and seems to work on disconnect but when you try to reconnect it just hangs on the loading screen without any error... Any idea ?

(Note: it does launch a new ipfs node on reconnect without any error, but stay on the loading screen)

haadcode commented 7 years ago

Left couple of comments for the code: need to refactor the flow a little more, see above for the comments on the startDaemon/init orbit/network.connect part.

I think with that fixed, we're close.

The other remaining big issue is the styling of the configuration screen. I saw you made some more changes to it which are close to Orbit's general style. Let's get the styling to a good state so that it doesn't pop up as much as it does now and we're good.

One more request, and you don't have to do that in this PR if you don't want to, would be to expose the Orbit's data dir to the configuration too. Same as Ipfs Data Directory but the one in OrbitConfig, so that we can say "Start Orbit using data from this directory" in order to isolate all instances properly (ie. running multiple Orbit's on the same machine).

theobat commented 7 years ago

Hi @haadcode, modified the flow with your code, refactored a few things and retested everything.
I am now working on the styling of the form, would you agree with using https://github.com/JedWatson/react-input-autosize to get a proper size for the path inputs ?

As far as this is concerned :

One more request, and you don't have to do that in this PR if you don't want to, would be to expose the Orbit's data dir to the configuration too. Same as Ipfs Data Directory but the one in OrbitConfig, so that we can say "Start Orbit using data from this directory" in order to isolate all instances properly (ie. running multiple Orbit's on the same machine).

I don't mind putting it in the same PR but the review might be a bit cumbersome in the end. Let me know your preference I'll adapt.

theobat commented 7 years ago

Stylesheet is ready as well now, you can review all the changes. :) (Without answer I chose not to add any dependency, thus the input's length is fixed for now)

haadcode commented 7 years ago

Excellent! I will review this today.

haadcode commented 7 years ago

Ok, went through the code and tested it. Here's my thoughts:

[ ] The Swarm Address and API settings in configuration still need styling.
[ ] API settings are missing a header
[ ] Use a smaller font for the form with class "IpfsSettingsView", so that the text fits on one line for all fields. For the API settings and Swarm Addresses, you can use <br/><input/> so that the header is on a different row, if needed.</p> </li> <li> <p>[ ] There's bug in the input fields in the configuration view where upon entering a new value, the cursor gets moved to the end of the line on every change. Looking at the code, it'd be better to not save the changes to IpfsDaemonStore on every change but when "save" is pressed. Keep the changes in the local state, ie. <code>this.state.ipfsDaemonSettings</code> and on save pass the full object to the store:</p> <pre><code class="language-javascript">save(e) { e ? e.preventDefault() : e; IpfsDaemonActions.persist(this.state.ipfsDaemonSettings); AppActions.setLocation('Connect'); }</code></pre> </li> <li> <p>As a side note, might be better to rename <code>IpfsDaemonActions.persist</code> to <code>IpfsDaemonActions.saveConfiguration</code> to make it more clear what that action does.</p> </li> <li> <p>It'd be great to have the true/false and API settings as checkboxes, but this is not required for this PR to get merged. Something for the future.</p> </li> </ul> <p>So overall we're very close. The main things are to 1) style the rest of the configuration form and 2) fix the bug regarding the input fields (this will also make the code simpler in IpfsSettingsView). After those are done, this is ready to merge I believe!</p> <p>Before you do the last commit, could you do me a favor an run <code>grunt build</code> in client/ (instead of npm run build), so that the dist/ gets updated properly. You should commit those files in, too.</p> <p>Let's leave out the Orbit data path and the request for checkboxes out of this PR and get it merged. We can work on polishing everything later. The main thing is, it works and brings value to the software already, so I don't want to get picky about small features, polish nor code style but rather merge it and refactor it later.</p> <p>Once this is merged, I'll need to do some major re-organization (moving the UI to its own repo in Github, eg. orbit-webapp or smth) and then start making it work in the browser only again. I'll probably have to refactor things all around the UI, so I hope you don't mind if I go and clean code from this PR as I clean things everywhere. I was thinking it might be good to put the Orbit instance to its own store also, not sure yet but something I might try. Plus a lot of small UI polish with colors, pixels, etc.</p> <p>How does that sound for the path forward here? Do you feel like you want to make a (pixel-)perfect PR or are you fine if we merge this in asap and polish/refactor later?</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/haadcode"><img src="https://avatars.githubusercontent.com/u/7499694?v=4" />haadcode</a> commented <strong> 7 years ago</strong> </div> <div class="markdown-body"> <p>(Trying to get this issue picked up by Waffle)</p> <p>Closes #166 </p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/theobat"><img src="https://avatars.githubusercontent.com/u/8222999?v=4" />theobat</a> commented <strong> 7 years ago</strong> </div> <div class="markdown-body"> <p>Ok I'll try to get everything done by this evening. I'm not sure how you'd like the styling though, especially for the ListForm components. Let's merge even if the styling is not pixel perfect. I do agree that true/false should get a checkbox, I misunderstood because it had a [ ] wrapping it (which is why I thought many values could be added, same goes for allow-access). Actually, it should probably <em>not</em> be a ListForm but we'll see...</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/haadcode"><img src="https://avatars.githubusercontent.com/u/7499694?v=4" />haadcode</a> commented <strong> 7 years ago</strong> </div> <div class="markdown-body"> <blockquote> <p>I'm not sure how you'd like the styling though, especially for the ListForm components.</p> </blockquote> <p>I think something similar to what the other input fields/labels are. Something like:</p> <pre><code><title>Directories</title> IPFS Data Path /path/to/ipfs <title>IPFS Addresses</title> API /ip4/127.0.0.1/tcp/0 Gateway /ip4/0.0.0.0/tcp/0 Swarm /ip4/0.0.0.0/tcp/0 /ip4/0.0.0.0/tcp/5001 <title>HTTP Headers</title> Access-Control-Allow-Origin * http://localhost:5001 Access-Control-Allow-Methods PUT GET Access-Control-Allow-Credentials true</code></pre> <p>This is just a proposed layout, you don't have to follow it. The main thing there is to make sure every option is descriptive (title) and that the labels and input fields match across the view.</p> <p>Does that help?</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/haadcode"><img src="https://avatars.githubusercontent.com/u/7499694?v=4" />haadcode</a> commented <strong> 7 years ago</strong> </div> <div class="markdown-body"> <p>And yes, let's merge even if it's not perfect :)</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/theobat"><img src="https://avatars.githubusercontent.com/u/8222999?v=4" />theobat</a> commented <strong> 7 years ago</strong> </div> <div class="markdown-body"> <p>hi @haadcode ! I feel we're done here, let me know what you think ;) </p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/haadcode"><img src="https://avatars.githubusercontent.com/u/7499694?v=4" />haadcode</a> commented <strong> 7 years ago</strong> </div> <div class="markdown-body"> <p>Gonna review this now, sorry for the delay.</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/haadcode"><img src="https://avatars.githubusercontent.com/u/7499694?v=4" />haadcode</a> commented <strong> 7 years ago</strong> </div> <div class="markdown-body"> <p>hey @theobat, this is gonna take me a little more time. need to get a clean merge with the dev branch that includes a bunch of big changes too and complex module dependencies. I'll be able to continue checking this tomorrow.</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/haadcode"><img src="https://avatars.githubusercontent.com/u/7499694?v=4" />haadcode</a> commented <strong> 7 years ago</strong> </div> <div class="markdown-body"> <p>Merged! \o/ 🎉 </p> <p>Thank you so much for the work you did for this PR @theobat! It wasn't an easy and there were multiple tricky pieces to it. This is a great new feature for Orbit and much needed.</p> <p>We're in the process of releasing IPFS 0.4.5 and Orbit is going to use it asap. So my next step is to start integrating that into this repo and in the process I'll refactor some of the startup/boot logic. I also want to add the Orbit Data Dir to the config, so I might touch that and perhaps polish it a little on the way.</p> <p>Is there something you'd like to work on next (improve this or something else)?</p> </div> </div> <div class="page-bar-simple"> </div> <div class="footer"> <ul class="body"> <li>© <script> document.write(new Date().getFullYear()) </script> Githubissues.</li> <li>Githubissues is a development platform for aggregating issues.</li> </ul> </div> <script src="https://cdn.jsdelivr.net/npm/jquery@3.5.1/dist/jquery.min.js"></script> <script src="/githubissues/assets/js.js"></script> <script src="/githubissues/assets/markdown.js"></script> <script src="https://cdn.jsdelivr.net/gh/highlightjs/cdn-release@11.4.0/build/highlight.min.js"></script> <script src="https://cdn.jsdelivr.net/gh/highlightjs/cdn-release@11.4.0/build/languages/go.min.js"></script> <script> hljs.highlightAll(); </script> </body> </html>

orbitdb-archive / orbit

[WIP] configuration ui for ipfs daemon #184

Simple

Composed (refactoring IpfsDaemonConfig)