query network version when metamask is back on line #3004
Conversation
app/scripts/controllers/network.js
Outdated
| log.info('web3.getNetwork returned ' + network) | ||
| this.setNetworkState(network) | ||
| cb(err, network) |
There was a problem hiding this comment.
if err is truthy, this part will not be reached
app/scripts/controllers/network.js
Outdated
| if (err) { | ||
| this.setNetworkState('loading') | ||
| console.error(err) | ||
| return |
app/scripts/controllers/network.js
Outdated
| @@ -58,15 +58,20 @@ module.exports = class NetworkController extends EventEmitter { | |||
| return this.getNetworkState() === 'loading' | |||
| } | |||
|
|
|||
| lookupNetwork () { | |||
| lookupNetwork (cb = () => {}) { | |||
There was a problem hiding this comment.
define noop up by the require statements
app/scripts/platforms/extension.js
Outdated
| return navigator.onLine | ||
| } | ||
|
|
||
| onLine (cb) { |
app/scripts/metamask-controller.js
Outdated
| // guard against premature firring of the event listener | ||
| if (this.provider) { | ||
| // start the provider up | ||
| this.provider.stop() |
There was a problem hiding this comment.
this is referring to the blockTracker actually, correct?
app/scripts/metamask-controller.js
Outdated
| // look up the network | ||
| this.networkController.lookupNetwork(() => { | ||
| this.emit('update', this.getState()) | ||
| }) |
There was a problem hiding this comment.
this is not the correct way to emit updates for MetamaskController
|
Related to #2548 |
| if (err) return this.setNetworkState('loading') | ||
| if (err) { | ||
| this.setNetworkState('loading') | ||
| console.error(err) |
There was a problem hiding this comment.
Should be log.error to use our loglevel utility.
| @@ -61,6 +61,21 @@ module.exports = class MetamaskController extends EventEmitter { | |||
| // network store | |||
| this.networkController = new NetworkController(initState.NetworkController) | |||
|
|
|||
| // setup onLine lister for restarting the provider | |||
| if (this.platform.addOnLineListener) { | |||
There was a problem hiding this comment.
"online" is one word, I'd rather we didn't camel case it into two. Non blocking.
| } | ||
|
|
||
| addOnLineListener (cb) { | ||
| window.addEventListener('online', cb) |
There was a problem hiding this comment.
Yeah camel cased correctly here. Incorrect capitalization kinda driving me nuts, going to change it myself.
|
I can readily reproduce an instance of #2636 on this branch with these steps:
Results: The network spinner stays on. The block tracker's Regression:If a test network is successfully connected, and then disconnected, the network indicator is never shown. This seems to me like it breaks the basic functionality of the network connection spinner, and is arguably worse than the current behavior. I wouldn't want to merge this until the spinner accurately reflects network status. |
| @@ -26,6 +26,14 @@ class ExtensionPlatform { | |||
| cb(e) | |||
| } | |||
| } | |||
|
|
|||
| isOnline () { | |||
| return navigator.onLine | |||
There was a problem hiding this comment.
sadly, this is the web api's camelization
|
@frankiebee said we can retire this one |
partial fix for issue #2636 however their maybe some other things in that issue that i am not considering.
the issue this fixes is that if a provider was created when offline the network spinner goes for ever and a request never goes through for net_version, this should fix that.
their are subsequent issues that have been discovered in the pursuit of fixing this one:
MetaMask/eth-block-tracker#27
#3003