Fix consistent-return issues #9192
Conversation
See [`consistent-return`](https://eslint.org/docs/rules/consistent-return) for more information. This change enables `consistent-return` and fixes the issues raised by the rule.
whymarrh
force-pushed
the
enable-consistent-return
branch
from
7087edd to
912ef2a
Compare
| @@ -327,7 +327,7 @@ function setupController (initState, initLangCode) { | |||
| const isMetaMaskInternalProcess = metamaskInternalProcessHash[processName] | |||
|
|
|||
| if (metamaskBlockedPorts.includes(remotePort.name)) { | |||
| return false | |||
| return | |||
There was a problem hiding this comment.
This is potentially functionally different. Why is this the correct way to fix this lint error?
There was a problem hiding this comment.
I suspect it's because the return value is not used by any caller.
There was a problem hiding this comment.
Because this function is only used as a listener for extension.runtime.onConnect:
extension.runtime.onConnect.addListener(connectRemote)And the return value from the event listener isn't conventionally important. It's also not documented as being used.[1]
| @@ -256,7 +256,8 @@ export default class AccountTracker { | |||
| ethContract.balances(addresses, ethBalance, (error, result) => { | |||
| if (error) { | |||
| log.warn(`MetaMask - Account Tracker single call balance fetch failed`, error) | |||
| return Promise.all(addresses.map(this._updateAccount.bind(this))) | |||
| Promise.all(addresses.map(this._updateAccount.bind(this))) | |||
There was a problem hiding this comment.
should there be an await here? the place where this function is called awaits, but without return a promise, doesn't this function call become non-blocking?
or maybe I don't know something about async await...
There was a problem hiding this comment.
There should but that's a functional change for a different PR. This works no worse/better than before.
(Also this is a common problem in the codebase and we should audit all uses of promises and async/await systematically.)
There was a problem hiding this comment.
but I mean, isn't
return Promise.all(addresses.map(this._updateAccount.bind(this)))
functionally different from
Promise.all(addresses.map(this._updateAccount.bind(this)))
return
?
There was a problem hiding this comment.
It's equivalent if the return value is unused. Since the function in question here is the callback being passed to ethContract.balances, the return value is not used. Nothing is await-ing this Promise.
There was a problem hiding this comment.
If the return value were used, yes, but it is not.
There was a problem hiding this comment.
But this is an easier change than ensuring the function continues to work when the timing semantics have changed. If we wait for the updates to happen that would be more different.
There was a problem hiding this comment.
" the function in question here is the callback being passed to"
doh, right, I should have realized that
Refs #8982
See
consistent-returnfor more information.This change enables
consistent-returnand fixes the issues raised by the rule.