feat: try more ways until failing & store provider info #5

hacdias · 2018-09-10T13:43:51Z

Signed-off-by: Henrique Dias <[email protected]>

olizilla · 2018-09-12T14:09:47Z

index.js

@@ -43,15 +29,15 @@ module.exports = () => {
      }

      if (type === 'IPFS_INIT_FINISHED') {
-        return Object.assign({}, state, { ready: true, identity: payload })
+        return Object.assign({}, state, { ready: true, ...payload })


I don't think we can use spread without adding a transpilation step to this repo

@olizilla it should work after 8.6.0 (https://node.green/#ES2018-features-object-rest-spread-properties) so I think it's okay.

I'm more thinking that we are publishing this as an npm module, and it's worth thinking about how people might want to consume it.

It's intended to be used in browsers rather than node, so it needs some thought.

@olizilla okay. I'll change that then 😄 I just saw the compatibility table on MDN and I really thought Edge and Safari worked with object spread but they don't!

olizilla

Please add tests. I'd like to see support setting ipfsApi via the url query as a separate PR; it's an unrelated issue, and it's worth discussion as it gives more power to the link author than is normal.

hacdias · 2018-09-12T16:07:20Z

@olizilla yeah, I just didn't add tests before to make sure I wasn't spending time with functionalities I'll not add here (link the link thingy).

License: MIT Signed-off-by: Henrique Dias <[email protected]>

hacdias · 2018-09-12T21:29:14Z

@olizilla this is a breaking change, as it removes selectIpfsApiAddress and replaces it by selectIpfsApiOpts. The same for doUpdateIpfsAPIAddress.

It also introduces js-ipfs-api as a dependency.
If the user wants to use js-ipfs, it is their responsibility to pass an instance to the bundle.

License: MIT Signed-off-by: Henrique Dias <[email protected]>

olizilla

Getting there. A few tweaks and some more tests needed.

We can avoid the breaking change by offering a selectIpfsApi method that converts the apiOprs into a multiaddr, and we can do the same for the doUpdateIpfsApi, which takes a multiaddr but stores it as an opts object. That'd be nice as that's the api the UI is using currently.

olizilla · 2018-09-13T08:44:21Z

README.md

+    // These are the defaults:
+    tryWindow: true,      // set false to bypass window.ipfs verification
+    tryApi: true,         // set false to bypass js-ipfs-api verification. Uses data from ipfsApi variable in localStorage
+    tryJsIpfs: false,     // set false to bypass js-ipfs-api verification. Uses data from ipfsOpts variable in localStorage


"set true to attempt js-ipfs initialisation"

olizilla · 2018-09-13T08:59:53Z

index.js

+  async function getIpfs (opts = {}, { getState, dispatch }) {
+    dispatch({ type: 'IPFS_INIT_STARTED' })
+
+    const leave = (provider, res, apiOpts) => {


I took me a while to figure out what leave meant in this case. Somthing like dispatchInitFinished would make it more obvious what this is for.

olizilla · 2018-09-13T09:13:16Z

README.md


- Updates the API Address to `address`.
+- Updates the API Options to `opts`.


Add a warning here that calling this will dispose of any existing ipfs instance and will only try to init an js-ipfs-api instance, and it won't try window.ipfs or js-ipfs.

olizilla · 2018-09-13T09:13:31Z

index.js

-      let identity = null
+    doUpdateIpfsApiOpts: (usrOpts) => (store) => {
+      store.dispatch({ type: 'IPFS_API_UPDATED', payload: usrOpts })
+      saveUserOpts('ipfsApi', usrOpts)


Actions tell us that something just happened, so the call to saveUserOpts should come before the dispatch, as the api isn't really updated until it's been stored.

Now we're dealing with an api opts object the action would be more accurately called IPFS_API_OPTS_UPDATED

olizilla · 2018-09-13T09:16:54Z

index.js

+    console.log('Trying ipfs-api', opts)
+
+    console.info('🎛️ Customise your js-ipfs-api options by setting:')
+    console.info('\t1. an address in the URL with `ipfsApi` param. e.g. ?ipfsApi=/ip4/127.0.0.1/tcp/5001')


Let's simplify this back down to "store an opts object". It gets messy having to deal with both types.

olizilla · 2018-09-13T09:17:36Z

index.js

+
+async function tryApi (opts) {
+  try {
+    console.time('IPFS_INIT_API')


I got carried away with the logging here, we can merge the timer logs and the "ready!" console message into one , by removing console.time('IPFS_INIT_API') and doing:

console.time('js-ipfs-api ready!') // ... stuff and magic console.timeEnd('js-ipfs-api ready!')

olizilla · 2018-09-13T09:18:26Z

index.js

+    console.info('🎛️ Customise your js-ipfs opts by setting an `ipfsOpts` value in localStorage. e.g. localStorage.setItem(\'ipfsOpts\', JSON.stringify({relay: {enabled: true}}))')
+    const ipfs = await initJsIpfs(Ipfs, opts)
+    const identity = await ipfs.id()
+    console.log('js-ipfs ready!')


Same as the console.log / console.time issue in tryApi

olizilla · 2018-09-13T09:25:24Z

index.test.js

 const ipfsBundle = require('./index')

-jest.mock('window.ipfs-fallback')
+it('Should fail when connecting via window.ipfs', (done) => {


Should fail to connect via window.ipfs when missing

olizilla · 2018-09-13T09:25:57Z

index.test.js

+  store.doInitIpfs()
+})
+
+it('Should success when connecting via window.ipfs', (done) => {


should connect via window.ipfs

olizilla · 2018-09-13T09:29:48Z

index.test.js

@@ -50,7 +68,7 @@ it('should initialise IPFS API by accessing it directly when running inside of i
  }

  const store = composeBundlesRaw(
-    ipfsBundle()
+    ipfsBundle({ tryApi: false, tryJsIpfs: false })
  )()



Please add tests for

Should connect via js-ipfs-api

Should connect via js-ipfs

Should connect via js-ipfs-api when window.ipfs present but disabled by options

Should connect via js-ipfs-api when window.ipfs present but fails

Should connect via js-ipfs instead of window.ipfs or js-ipfs-api

License: MIT Signed-off-by: Henrique Dias <[email protected]>

hacdias · 2018-09-14T11:10:10Z

@olizilla apparently Travis liked the tests 😄

olizilla · 2018-09-14T14:36:54Z

index.test.js

+    store.subscribeToSelectors(['selectIpfsReady'], () => {
+      if (first) {
+        expect(store.selectIpfsReady()).toBe(true)
+        expect(store.selectIpfsIdentity()).not.toEqual(testIdentity)


Testing that the identity is not equal to the test one doesn't tell us very much. It'd be useful to test for the value of selectIpfsProvider is set correctly in all these cases.

olizilla · 2018-09-14T14:49:53Z

How about making this not a breaking change by pulling the logic you have in https://github.com/ipfs-shipyard/ipfs-webui/pull/792/files#diff-1d5a0de57bad3136d70286a7887c1a8bR20 to map between the object form and the multi addr form into this bundle.

We can store the info as an object and offer a selector that formats the object as a multiaddr, and a action creator that lets the user update the api opts object by specifying a multiaddr.

License: MIT Signed-off-by: Henrique Dias <[email protected]>

hacdias · 2018-09-14T20:51:35Z

@olizilla done 😄

License: MIT Signed-off-by: Henrique Dias <[email protected]>

olizilla

The multiaddr logic needs more thought.

olizilla · 2018-09-17T08:53:39Z

index.js

+      store.doUpdateIpfsApiOpts({
+        host: addr[2],
+        port: addr[4],
+        protocol: 'http'


We need to check the multi addr to see if it specifies https rather than assuming http

olizilla · 2018-09-17T08:54:19Z

index.js


-    selectIpfsInitFailed: state => !!state.ipfs.error,
+    selectIpfsApiAddress: state => `/ip4/${state.ipfs.apiOpts.host}/tcp/${state.ipfs.apiOpts.port}`,


We need to figure out if this is an ip4 or ip6 host from the host value.

olizilla · 2018-09-17T08:55:59Z

I'll merge this PR, and raise an issue for the multiaddr handling.

hacdias · 2018-09-17T08:56:22Z

Awesome! And could you release a new version too?

hacdias added 2 commits September 10, 2018 14:43

wip: try-more-ways-until-failing

84a046e

License: MIT

2d3c754

Signed-off-by: Henrique Dias <[email protected]>

olizilla reviewed Sep 12, 2018

View reviewed changes

olizilla suggested changes Sep 12, 2018

View reviewed changes

hacdias added 5 commits September 12, 2018 17:22

remove url integration

bb16ba1

License: MIT Signed-off-by: Henrique Dias <[email protected]>

fix: linting and cleansing

0ac19c6

License: MIT Signed-off-by: Henrique Dias <[email protected]>

add tests and fix issues

5b520fa

License: MIT Signed-off-by: Henrique Dias <[email protected]>

fixes

f520209

License: MIT Signed-off-by: Henrique Dias <[email protected]>

docs: add new API

0cf338d

License: MIT Signed-off-by: Henrique Dias <[email protected]>

fix docs

9646e53

License: MIT Signed-off-by: Henrique Dias <[email protected]>

hacdias changed the title ~~wip: try more ways until failing & store provider info~~ feat(BREAKING CHANGE): try more ways until failing & store provider info Sep 12, 2018

fix: bug

f89c538

License: MIT Signed-off-by: Henrique Dias <[email protected]>

hacdias mentioned this pull request Sep 12, 2018

[wip] feat: use new version of ipfs redux bundle ipfs/ipfs-webui#792

Closed

olizilla suggested changes Sep 13, 2018

View reviewed changes

hacdias added 5 commits September 14, 2018 10:26

wip: tweaks

b697174

License: MIT Signed-off-by: Henrique Dias <[email protected]>

feat: more testing

fd85878

License: MIT Signed-off-by: Henrique Dias <[email protected]>

js-ipfs tests

607d82d

License: MIT Signed-off-by: Henrique Dias <[email protected]>

docs: add doStopIpfs

7d0a8af

License: MIT Signed-off-by: Henrique Dias <[email protected]>

add some long timeouts

81473f9

License: MIT Signed-off-by: Henrique Dias <[email protected]>

olizilla reviewed Sep 14, 2018

View reviewed changes

hacdias added 2 commits September 14, 2018 21:45

verify provider ont ests

df17abb

License: MIT Signed-off-by: Henrique Dias <[email protected]>

add selectIpfsApiAddress and doUpdateIpfsApiAddress

6751aa2

License: MIT Signed-off-by: Henrique Dias <[email protected]>

hacdias changed the title ~~feat(BREAKING CHANGE): try more ways until failing & store provider info~~ feat: try more ways until failing & store provider info Sep 14, 2018

hacdias added 2 commits September 14, 2018 21:59

fix bugs

4b528a4

License: MIT Signed-off-by: Henrique Dias <[email protected]>

feat: Ipfs --> getIpfs to allow dynamic importing

52430af

License: MIT Signed-off-by: Henrique Dias <[email protected]>

olizilla reviewed Sep 17, 2018

View reviewed changes

olizilla merged commit 82a6e03 into master Sep 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: try more ways until failing & store provider info #5

feat: try more ways until failing & store provider info #5

hacdias commented Sep 10, 2018 •

edited

Loading

olizilla Sep 12, 2018

hacdias Sep 12, 2018 •

edited

Loading

olizilla Sep 12, 2018

hacdias Sep 12, 2018

olizilla left a comment

hacdias commented Sep 12, 2018

hacdias commented Sep 12, 2018 •

edited

Loading

olizilla left a comment

olizilla Sep 13, 2018

olizilla Sep 13, 2018

olizilla Sep 13, 2018

olizilla Sep 13, 2018

olizilla Sep 13, 2018

olizilla Sep 13, 2018

olizilla Sep 13, 2018

olizilla Sep 13, 2018

olizilla Sep 13, 2018

olizilla Sep 13, 2018

hacdias commented Sep 14, 2018

olizilla Sep 14, 2018

olizilla commented Sep 14, 2018

hacdias commented Sep 14, 2018

olizilla left a comment

olizilla Sep 17, 2018

olizilla Sep 17, 2018

olizilla commented Sep 17, 2018

hacdias commented Sep 17, 2018


		- Updates the API Address to `address`.
		- Updates the API Options to `opts`.


		selectIpfsInitFailed: state => !!state.ipfs.error,
		selectIpfsApiAddress: state => `/ip4/${state.ipfs.apiOpts.host}/tcp/${state.ipfs.apiOpts.port}`,

feat: try more ways until failing & store provider info #5

feat: try more ways until failing & store provider info #5

Conversation

hacdias commented Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

hacdias Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olizilla left a comment

Choose a reason for hiding this comment

hacdias commented Sep 12, 2018

hacdias commented Sep 12, 2018 • edited Loading

olizilla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacdias commented Sep 14, 2018

Choose a reason for hiding this comment

olizilla commented Sep 14, 2018

hacdias commented Sep 14, 2018

olizilla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olizilla commented Sep 17, 2018

hacdias commented Sep 17, 2018

hacdias commented Sep 10, 2018 •

edited

Loading

hacdias Sep 12, 2018 •

edited

Loading

hacdias commented Sep 12, 2018 •

edited

Loading