UX fixes around embedded js-ipfs and automatic mode #381

lidel · 2018-02-11T18:31:49Z

I started dog-fooding embedded mode in Firefox and fixed some UX bugs related to it.

suspend redirect to custom gateway (and automatic mode) when embedded node is active
redirect to public gateway instead of custom one when running js-ipfs

This works fine in Firefox/Chrome, but may need additional work in separate PR to get optimal UX under Brave (eg. we probably want to redirect IPFS resources to ipfs:// instead of keeping URL of public gateway).
fix UX when starting with external API being offline (close Daemon is off but redirect is still enabled #373)
extract some reusable feature-detection functions
open public gateway instead of custom one when uploading a file
using embedded js-ipfs under Firefox
separate redirect tests for embedded and external modes
make it work under Chrome
Pinning does not work on Firefox when using js-ipfs node (close Pinning does not work on Firefox when using js-ipfs node #380)
Move runtime feature detection to runtime-checks.js + tests

Known Issues (to be addressed in separate PR)

Rube Goldberg machine-like workflow breaks browser action menu Rube Goldberg machine-like workflow breaks browser action menu #388

lidel · 2018-02-11T20:29:42Z

I was able to make it work in Chrome by adding const browser = require('webextension-polyfill') to add-on/src/lib/state.js but I get ReferenceError: chrome is not defined when I run test suite (yarn test).

@alanshaw, @olizilla did you see this error before by any chance?

alanshaw · 2018-02-12T10:02:42Z

I've not seen that before. Stack trace please?

lidel · 2018-02-12T10:38:28Z

@alanshaw I added mentioned change to this PR, full stacktrace is at Jenkins: PR-381/3/pipeline#step-7-log-10.

Perhaps I should just refactor/move feature detection to separate module?

lidel · 2018-02-19T13:27:51Z

Fixed it by extracting feature detection code into feature-detector.js (0cc1ad1)
I don't like the name, but does the job + is finally testable.

Do you see anything that could be improved before we merge this?

alanshaw · 2018-02-19T14:36:02Z

I'll take a look at these as soon as I can

alanshaw · 2018-02-20T09:49:12Z

Next time could you split changes for multiple issues like this into multiple PRs? It's much easier for me when reviewing to see which changes relate to which issues when they're separate.

lidel · 2018-02-20T10:17:14Z

Yeah, my bad. I realized there is a size problem around 4th "fix" in this PR.
It was supposed to be much smaller. 😞

alanshaw · 2018-02-20T12:55:35Z

add-on/src/lib/feature-detector.js

+  return {
+    getState () {
+      return getState()
+    },


I'd rather not duplicate this if possible - what's the reasoning behind it?

alanshaw · 2018-02-20T13:35:09Z

add-on/src/lib/feature-detector.js

+    inBrowserWithNativeProtocol () {
+      return runtimeHasNativeProtocol
+    }
+  }


Would these be better expressed as things that this browser is and has, so:

inFirefox => isFirefox

inAndroid => isAndroid

inBrowserWithNativeProtocol => hasProtocolHandler (or whatever)

Do these need to be functions? They're not performing any computation. I'd be tempted to just return a frozen object with the computed values.

embeddedNodeIsActive feels like the odd one out here, it's not a feature of the browser and is easily detected. I'd vote for removing it from here and simply performing this check inline.

alanshaw · 2018-02-20T13:41:50Z

add-on/src/lib/ipfs-companion.js

-    if (state.ipfsNodeType === 'embedded' && browser && browser.protocol && browser.protocol.registerStringProtocol) {
-      return {path, localAddress: `ipfs://${hash}`}
+    if (featureDetector.embeddedNodeIsActive() && featureDetector.inBrowserWithNativeProtocol()) {
+      return {path, nativeAddress: `ipfs://${hash}`}


Just {path, url: `ipfs://${hash}`}?

alanshaw · 2018-02-20T13:42:03Z

add-on/src/lib/ipfs-companion.js

-      return {path, localAddress: url}
+      // open at public GW (will be redirected to local elsewhere, if enabled)
+      const url = new URL(path, state.pubGwURLString).toString()
+      return {path, nativeAddress: url}


Just {path, url}?

alanshaw · 2018-02-20T13:43:17Z

add-on/src/lib/ipfs-companion.js

    const path = `/ipfs/${hash}`
-    if (state.ipfsNodeType === 'embedded' && browser && browser.protocol && browser.protocol.registerStringProtocol) {
-      return {path, localAddress: `ipfs://${hash}`}
+    if (featureDetector.embeddedNodeIsActive() && featureDetector.inBrowserWithNativeProtocol()) {


I appreciate this isn't your change, but why does the node type matter here? ping @olizilla?

Good question. Logically, if we've been able to registerStringProtocol handler then ipfs:// addresses should work. I think when I wrote this I was only using the embedded js-ipfs node in the registerStringProtocol handler function, but now we support using either. It should be safe to remove the node type test.

alanshaw · 2018-02-20T14:04:46Z

add-on/src/lib/ipfs-request.js

@@ -43,7 +49,7 @@ function createRequestModifier (getState, dnsLink, ipfsPathValidator) {
      }
      // Detect valid /ipfs/ and /ipns/ on any site
      if (ipfsPathValidator.publicIpfsOrIpnsResource(request.url)) {
-        return redirectToCustomGateway(request.url, state.gwURL)
+        return redirectToIpfsTransport(request.url, state, featureDetector)


I don't know what a IpfsTransport is, how about just redirectToGateway (could be custom or public)

alanshaw · 2018-02-20T14:07:36Z

add-on/src/lib/ipfs-request.js

-function redirectToCustomGateway (requestUrl, gwUrl) {
+function redirectToIpfsTransport (requestUrl, state, featureDetector) {
+  // TODO: redirect to `ipfs://` if inBrowserWithNativeProtocol() === true
+  const gwUrl = featureDetector.embeddedNodeIsActive() ? state.pubGwURL : state.gwURL


Nitpick but, I vote to keep this function simple, as it was, and perform the feature detection before it is called.

lidel · 2018-02-20T16:06:53Z

@alanshaw simplified things just like you sugested in #381 (review):

state-based checks are inlined
remaining runtime checks are no longer functions but precomputed booleans

Ok to merge?

alanshaw · 2018-02-20T16:13:03Z

🚀 👍

- suspend automatic mode while embedded node is active - redirect to public gateway instead of custom one when running js-ipfs - fix UX when starting with external API being offline (#373) - extract some reusable feature-detection functions - open public gateway instead of custom one when uploading a file using embedded js-ipfs under Firefox - some tests

TODO: fix tests

- there's no pinning in js-ipfs yet - closes #380

- platform and browser vendor features are detected once and cached - easier to test

- state-based checks are inlined - remaining runtime checks are no longer functions, but precomputed booleans

lidel added the UX label Feb 11, 2018

lidel added this to the 2018-Q1 milestone Feb 11, 2018

lidel self-assigned this Feb 11, 2018

lidel force-pushed the fix/automatic-mode-when-start-offline branch from e6eb2ba to 53ef74a Compare February 11, 2018 19:27

lidel requested review from alanshaw and olizilla and removed request for alanshaw February 11, 2018 19:34

lidel changed the title ~~UX fixes around embedded js-ipfs and automatic mode~~ [WIP] UX fixes around embedded js-ipfs and automatic mode Feb 11, 2018

lidel force-pushed the fix/automatic-mode-when-start-offline branch from 6d679b9 to 0cc1ad1 Compare February 19, 2018 12:51

lidel changed the title ~~[WIP] UX fixes around embedded js-ipfs and automatic mode~~ UX fixes around embedded js-ipfs and automatic mode Feb 19, 2018

lidel mentioned this pull request Feb 19, 2018

Rube Goldberg machine-like workflow breaks browser action menu #388

Open

alanshaw suggested changes Feb 20, 2018

View reviewed changes

alanshaw approved these changes Feb 20, 2018

View reviewed changes

lidel added 5 commits February 20, 2018 17:13

Fix for Chrome

aff32ba

TODO: fix tests

Hide pinning controls when embedded js-ipfs is active

267034b

- there's no pinning in js-ipfs yet - closes #380

Move runtime feature detection to feature-detector.js

070e0ba

- platform and browser vendor features are detected once and cached - easier to test

Simplify/inline runtime checks

bd3368d

- state-based checks are inlined - remaining runtime checks are no longer functions, but precomputed booleans

lidel force-pushed the fix/automatic-mode-when-start-offline branch from 1d29651 to bd3368d Compare February 20, 2018 16:18

lidel merged commit f91107d into master Feb 20, 2018

lidel deleted the fix/automatic-mode-when-start-offline branch February 20, 2018 16:29

lidel mentioned this pull request Feb 22, 2018

Experiment: use js-ipfs as a fallback for uploading content when IPFS API is down #304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UX fixes around embedded js-ipfs and automatic mode #381

UX fixes around embedded js-ipfs and automatic mode #381

lidel commented Feb 11, 2018 •

edited

Loading

lidel commented Feb 11, 2018

alanshaw commented Feb 12, 2018 •

edited

Loading

lidel commented Feb 12, 2018 •

edited

Loading

lidel commented Feb 19, 2018

alanshaw commented Feb 19, 2018

alanshaw commented Feb 20, 2018

lidel commented Feb 20, 2018

alanshaw Feb 20, 2018

alanshaw Feb 20, 2018

alanshaw Feb 20, 2018

alanshaw Feb 20, 2018

alanshaw Feb 20, 2018

olizilla Feb 20, 2018 •

edited

Loading

alanshaw Feb 20, 2018

alanshaw Feb 20, 2018

lidel commented Feb 20, 2018

alanshaw commented Feb 20, 2018

UX fixes around embedded js-ipfs and automatic mode #381

UX fixes around embedded js-ipfs and automatic mode #381

Conversation

lidel commented Feb 11, 2018 • edited Loading

Known Issues (to be addressed in separate PR)

lidel commented Feb 11, 2018

alanshaw commented Feb 12, 2018 • edited Loading

lidel commented Feb 12, 2018 • edited Loading

lidel commented Feb 19, 2018

alanshaw commented Feb 19, 2018

alanshaw commented Feb 20, 2018

lidel commented Feb 20, 2018

alanshaw Feb 20, 2018

Choose a reason for hiding this comment

alanshaw Feb 20, 2018

Choose a reason for hiding this comment

alanshaw Feb 20, 2018

Choose a reason for hiding this comment

alanshaw Feb 20, 2018

Choose a reason for hiding this comment

alanshaw Feb 20, 2018

Choose a reason for hiding this comment

olizilla Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

alanshaw Feb 20, 2018

Choose a reason for hiding this comment

alanshaw Feb 20, 2018

Choose a reason for hiding this comment

lidel commented Feb 20, 2018

alanshaw commented Feb 20, 2018

lidel commented Feb 11, 2018 •

edited

Loading

alanshaw commented Feb 12, 2018 •

edited

Loading

lidel commented Feb 12, 2018 •

edited

Loading

olizilla Feb 20, 2018 •

edited

Loading