Allows embedded IPFS node to be configured #395

alanshaw · 2018-02-22T14:58:21Z

resolves #394

Simply adds a field in the options (if node type is embedded) that allows you to paste some JSON config. This allows the savvy user to configure their js-ipfs node as they like.

I've also removed the is-js-ipfs-enabled module as it is always enabled now (sorry that should probably be in another PR but is a small change so hopefully not too distracting).

This PR also upgrades choo to fix this problem with textareas: choojs/choo#632

lidel

👌 for adding this, but linting (npm run lint:web-ext) does not pass:

Code             Message             Description                          File                 Line   Column
FILE_TOO_LARGE   File is too large   This file is not binary and is too   dist/background/b…                
                 to parse.           large to parse. Files larger than    ackground.js                      
                                     4MB will not be parsed. If your                                        
                                     JavaScript file has a large list,                                      
                                     consider removing the list and                                         
                                     loading it as a separate JSON file                                     
                                     instead.

alanshaw · 2018-02-23T09:35:52Z

@lidel can we deal with the linting warning as a separate issue please?

alanshaw · 2018-02-23T09:36:39Z

@lidel @olizilla I added this because I needed to enable pubsub on the js-ipfs node, but this is real power user stuff and is error prone for new users. Would it be better to just have a checkbox to enable pubsub instead and leave the JSON config to the power users who are probably running their own daemon anyway?

lidel · 2018-02-23T10:31:09Z

Linter

Rule of thumb: we should never break build of master. It introduces friction for other people that want to work on own PRs.

I propose we park this PR until linting is fixed in separate PR, and then you can rebase this PR on top of that.

JSON and UX

Indeed, a field with JSON in it is error prone.
But I am not sure if we want to create a precedent of adding control for pubsub.

Let's keep JSON-based config, but make sure the "Reset Everything" button restores config to safe defaults.
That way of user breaks it, there is a way of fixing it.

We should also either add "(for Power Users)" to label description, or move it to "Experiments" section.

olizilla · 2018-02-23T10:54:48Z

On the subject of config... I say keep it simple. It's be helpful as a dev to know that I can build an app that will run if the user installs ipfs-companion as it has a fairly predictable config. They don't have to install a local go node or desktop, the defaults in companion will work.

I'm not sure who we are catering for in making an easy to use embedded node also be "power-user" configurable. A simpler story would be "embedded node with defaults" for the casual user, "external node with custom config" or just, "create an app that bundles it's own custom ipfs node" if your app or use case has custom requirements.

I'd be happy to start in that direction, and then see if anyone starts demanding more configurability of the embedded node and review.

alanshaw · 2018-02-23T11:11:19Z

@lidel I'm happy for this PR to be blocked by the lint issue 👍

I appreciate that I opened this PR but I agree with @olizilla on this one - I think the stories should be casual -> embedded, expert -> external. I think JSON config is at odds with the casual embedded story.

Here's the thing. If we don't have JSON config and we don't have an enable pubsub checkbox then people can't use pubsub with window.ipfs.

Why?

Pubsub is disabled in js-ipfs-api when used in browsers (see ipfs-inactive/js-ipfs-http-client#493). If you run your own daemon configured to pubsub then we have to use js-ipfs-api.

Embedded js-ipfs is not configured to enable the pubsub experiment by default.

lidel · 2018-02-23T14:07:59Z

Hm.. now that I think about it, I'd say pubsub is marked as an experiment by js-ipfs for a reason :)

I propose we keep the "embedded node JSON config field" field in current form, but move it to to "Experiment" section (and update label to explicitly state that the config is for embedded node only).

That way that there will be no confusion that this feature is an opt-in experiment that can changed or removed in the future.

alanshaw · 2018-02-26T12:15:01Z

@lidel rebased and lint error is now gone 😄

lidel

I think this requires additional error handling.

For example, what if user sets:

{
  "config": {
    "Addresses": {
      "Swarm": [ "rekt" ]
    }
  }
}

According to my tests it is impossible to fix this right now, even via Reset button :(

Perhaps every use of initIpfsClient in onStorageChange should be wrapped by something like:

  async function safeInitIpfsClient (state) {
    try {
      // in theory someone can set invalid config that breaks js-ipfs
      // or we can introduce experiment that breaks things
      // this wrapper makes sure it does not fail silently
      return await initIpfsClient(state)
    } catch (error) {    
      console.error('Unable to initialize API client due to error', error)
      if (notify) notify('notify_addonIssueTitle', 'notify_addonIssueMsg')
      // TODO:  [restore old config value and update state object]
      return await initIpfsClient(state)
    }
  }

Did not test if that is enough tho.

lidel · 2018-02-26T15:33:05Z

Not sure if related to this PR, but also found: #407

alanshaw · 2018-02-26T20:04:13Z

Ok so, you're saying that "rekt" as a swarm value throws when we create a new IPFS instance?

We can code around that particular issue, but it's going to be difficult to guard against all config issues. I can imagine a situation where users accidentally stop their node from communicating on the network or even cause it to error when particular methods are called due to the config they've copy/pasted from the internet or changed themselves. I don't think that's something we can, or even should be trying to guard against...like I said above, it's error prone and I think a checkbox to enable pubsub would be a safer option for casual users.

As a side note, is the reset button not working for you? It should reset back to the default config.

lidel · 2018-02-26T22:41:00Z

I agree we can't (and we should not) guard against broken config.
My point was that Reset button does not work as expected (the real expectation is "make it all work again"). I am sorry, should emphasize that better :)

Replication steps

Set config with invalid "Swarm": [ "rekt" ] → node breaks
Press Reset Everything → the config is restored to defaults
(Swarm value on Preferences screen is restored to [ ] )
.. but the node does not work. Even if you toggle between embedded and external node API client does not start correctly (logs) and user is left with broken extension without a way of fixing it.

I would really like to keep this "full config" feature, as it encourages playful experimentation, but we need to make it possible to self-heal.

alanshaw · 2018-02-27T09:05:35Z

@lidel ok I'm on it! 👍

btw, how do you format a button in gfm?

lidel · 2018-02-27T11:02:31Z

<kbd>K</kbd> → K :)

alanshaw · 2018-02-27T11:43:54Z

Blocked by ipfs/js-ipfs#1193 (comment)

cvan · 2018-03-14T08:33:27Z

@alanshaw hi there! it looks like this won't be blocked anymore thanks to your work in PR ipfs/js-ipfs#1239 (though not upstreamed to a patch release yet). just wanted to make sure I'm following everything correctly. is that all correct? nice work, btw!

alanshaw · 2018-03-14T10:17:53Z

Thanks, I think we're just waiting on it to be released now @cvan

alanshaw · 2018-03-21T11:10:14Z

@lidel I've taken another look at this, updated the IPFS dependency and fixed the stability issues you were seeing.

There is now validation of config and feedback to the user and there's always a way to reset config and the extension acts gracefully when it fails to start an IPFS node.

I'm happy for this to merge if you are.

lidel

Validation works perfect now, I was unable to brick it 👍

If you fix i18 key names and move config to the old place, I am ok to merge it.

lidel · 2018-03-21T13:06:13Z

add-on/_locales/en/messages.json

@@ -169,6 +169,14 @@
    },
    "description": "A generic placeholder for error notification (notify_inlineErrorMsg)"
  },
+  "notify_startIpfsNodeTitle": {
+    "message": "Failed to start IPFS node",


We should change key names to reflect that message is for a failure:

notify_startIpfsNodeTitle

notify_stopIpfsNodeTitle

ha, oops I didn't mean to not indicate it was for a failure!

lidel · 2018-03-21T13:42:45Z

add-on/src/options/forms/experiments-form.js

@@ -87,6 +90,17 @@ function experimentsForm ({
          </label>
          <input type="checkbox" id="ipfs-proxy" onchange=${onIpfsProxyChange} checked=${ipfsProxy} />
        </div>
+        ${ipfsNodeType === 'embedded' ? html`
+          <div>
+            <label for="ipfsNodeConfig">


Now that we can safely recover any misconfiguration, lets move this back to the old place – below node type picker :)

alanshaw · 2018-03-22T12:06:25Z

@lidel looks good to you?

lidel · 2018-03-22T14:10:04Z

👌
Thanks!

alanshaw requested review from olizilla and lidel February 22, 2018 14:58

lidel requested changes Feb 23, 2018

View reviewed changes

alanshaw added the status/blocked Unable to be worked further until needs are met label Feb 23, 2018

alanshaw removed the status/blocked Unable to be worked further until needs are met label Feb 26, 2018

alanshaw force-pushed the feat/embedded-ipfs-config branch 2 times, most recently from 3bcfdc2 to c2a75c1 Compare February 26, 2018 12:14

lidel requested changes Feb 26, 2018

View reviewed changes

lidel mentioned this pull request Feb 26, 2018

Error: Not able to stop from state: starting #407

Closed

lidel added status/blocked Unable to be worked further until needs are met status/blocked/upstream-bug Blocked by upstream bugs labels Feb 27, 2018

alanshaw mentioned this pull request Feb 28, 2018

feat: add config validation ipfs/js-ipfs#1239

Merged

alanshaw added 3 commits March 21, 2018 09:05

allows embedded IPFS node to be configured

53f30df

init node config in state

c6f779b

moves config field to experiments section

464fe9a

adds error handling when restarting ipfs node with new config

16fe8d7

alanshaw force-pushed the feat/embedded-ipfs-config branch from f1fc161 to 16fe8d7 Compare March 21, 2018 09:08

updates IPFS and fixes stability issues

b9bc732

fixes message key in i18n

04f6c61

alanshaw removed status/blocked Unable to be worked further until needs are met status/blocked/upstream-bug Blocked by upstream bugs labels Mar 21, 2018

lidel requested changes Mar 21, 2018

View reviewed changes

alanshaw added 2 commits March 22, 2018 11:48

fixes i18n notification messages

6e2a790

moves the node config back under the node type field

ae4b179

lidel approved these changes Mar 22, 2018

View reviewed changes

lidel merged commit 19a6922 into ipfs:master Mar 22, 2018

olizilla deleted the feat/embedded-ipfs-config branch March 22, 2018 14:23

lidel mentioned this pull request Apr 13, 2018

Should embedded node use js-libp2p-websocket-star? #457

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows embedded IPFS node to be configured #395

Allows embedded IPFS node to be configured #395

alanshaw commented Feb 22, 2018 •

edited

Loading

lidel left a comment

alanshaw commented Feb 23, 2018

alanshaw commented Feb 23, 2018

lidel commented Feb 23, 2018

olizilla commented Feb 23, 2018

alanshaw commented Feb 23, 2018 •

edited

Loading

lidel commented Feb 23, 2018 •

edited

Loading

alanshaw commented Feb 26, 2018

lidel left a comment •

edited

Loading

lidel commented Feb 26, 2018

alanshaw commented Feb 26, 2018

lidel commented Feb 26, 2018

alanshaw commented Feb 27, 2018

lidel commented Feb 27, 2018

alanshaw commented Feb 27, 2018

cvan commented Mar 14, 2018 •

edited

Loading

alanshaw commented Mar 14, 2018

alanshaw commented Mar 21, 2018

lidel left a comment

lidel Mar 21, 2018

alanshaw Mar 22, 2018

lidel Mar 21, 2018

alanshaw commented Mar 22, 2018

lidel commented Mar 22, 2018

Allows embedded IPFS node to be configured #395

Allows embedded IPFS node to be configured #395

Conversation

alanshaw commented Feb 22, 2018 • edited Loading

lidel left a comment

Choose a reason for hiding this comment

alanshaw commented Feb 23, 2018

alanshaw commented Feb 23, 2018

lidel commented Feb 23, 2018

Linter

JSON and UX

olizilla commented Feb 23, 2018

alanshaw commented Feb 23, 2018 • edited Loading

lidel commented Feb 23, 2018 • edited Loading

alanshaw commented Feb 26, 2018

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel commented Feb 26, 2018

alanshaw commented Feb 26, 2018

lidel commented Feb 26, 2018

Replication steps

alanshaw commented Feb 27, 2018

lidel commented Feb 27, 2018

alanshaw commented Feb 27, 2018

cvan commented Mar 14, 2018 • edited Loading

alanshaw commented Mar 14, 2018

alanshaw commented Mar 21, 2018

lidel left a comment

Choose a reason for hiding this comment

lidel Mar 21, 2018

Choose a reason for hiding this comment

alanshaw Mar 22, 2018

Choose a reason for hiding this comment

lidel Mar 21, 2018

Choose a reason for hiding this comment

alanshaw commented Mar 22, 2018

lidel commented Mar 22, 2018

alanshaw commented Feb 22, 2018 •

edited

Loading

alanshaw commented Feb 23, 2018 •

edited

Loading

lidel commented Feb 23, 2018 •

edited

Loading

lidel left a comment •

edited

Loading

cvan commented Mar 14, 2018 •

edited

Loading