Save Brave Fixes #4270 #4524
Save Brave Fixes #4270 #4524
Conversation
|
|
||
| // safeguard for Brave Browser until they implement chrome.i18n.getAcceptLanguages | ||
| // https://github.com/MetaMask/metamask-extension/issues/4270 | ||
| if(!userPreferredLocaleCodes){ |
There was a problem hiding this comment.
Can I catch it? Not really; it's not raised by your code, but the cleanup code that handles async tasks in Chrome API; therefore using try ... catch in your callback won't help. Since it's asynchronous, using try around the original API call won't help either.
shoutouts to: https://stackoverflow.com/questions/28431505/unchecked-runtime-lasterror-when-using-chrome-api
| @@ -2,8 +2,7 @@ const extension = require('extensionizer') | |||
| const promisify = require('pify') | |||
| const allLocales = require('../../_locales/index.json') | |||
|
|
|||
| const isSupported = extension.i18n && extension.i18n.getAcceptLanguages | |||
| const getPreferredLocales = isSupported ? promisify( | |||
| const getPreferredLocales = extension.i18n ? promisify( | |||
There was a problem hiding this comment.
My understanding is that Brave does expose a extension.i18n object, and even a getAcceptLanguages method on it, so they would pass this test.
From what they reported and I relayed in this comment, the only way to check their failure right now is to check chrome.runtime.lastError within the callback, and if there's an error, then fall back.
There was a problem hiding this comment.
Yes, both evaluate successfully for Brave. Do you think it makes sense to keep the extension.i18n check for protection in browsers that don't expose i18n at all?
(am not sure how prevalent this is/would be)
Checking for the runtime.lastError did not seem to resolve, but possibly due to how it was wrapped in pify - I'll take another look.
There was a problem hiding this comment.
Per feedback from Brave, we are now handling errors from this function, because Brave currently throws for this.
|
Without evaluating the runtime.lastError, Brave returns undefined so the proposed change does fix the issue. |
|
@jennypollack Would you mind adding a user-facing entry to the |
CHANGELOG.md
Outdated
| @@ -3,6 +3,7 @@ | |||
| ## Current Master | |||
|
|
|||
| - Fix bug where account reset did not work with custom RPC providers. | |||
| - Fix for Brave i18n getAcceptLanguages [#4270](https://github.com/MetaMask/metamask-extension/issues/4270) | |||
There was a problem hiding this comment.
@jennypollack we add changelog entries under "current master" and then move that to a version number of release
|
From Brave Sampson:
|
In Brave Browser extension.i18n.getAcceptLanguages throws:
"Unchecked runtime.lastError while running i18n.getAcceptLanguages: Access to extension API denied."
Once Brave Browser implementation lands for locale support, no further change will be needed in MM to support language preferences.
Meanwhile this check should allow MetaMask to load on Brave.