Detect locale updated #501
Conversation
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
very cool, learned a lot about electron haha.
nice refactor as well! i like the addition of the action class + the tests look great :)
It was hard for me to actually test the packaged app since i'm in the US which is the default, but I'm planning to set up a VPN soon which would enable me to test the locale detection more robustly. But if it's working for you in production, I'm good to merge! :)
| expect(logger.error, 'was called with', /Detecting/, /Boom/); | ||
| }); | ||
|
|
||
| it('should fall back to USD for unsupported fiat', async () => { |
| const filter = event => { | ||
| if (!/^(lnd)|(unlock)|(log)|(open-url)[a-zA-Z_-]{0,20}$/.test(event)) { | ||
| if ( | ||
| !/^(lnd)|(unlock)|(log)|(locale)|(open-url)[a-zA-Z_-]{0,20}$/.test(event) |
There was a problem hiding this comment.
is this a security measure?
| * @fileOverview a low level action to wrap electron's IPC renderer api. | ||
| */ | ||
|
|
||
| class IpcAction { |
There was a problem hiding this comment.
this is nice 👌
learned a lot about electron reading this code!
| * Triggered the first time the app was started e.g. to set the | ||
| * local fiat currency only once. | ||
| */ | ||
| observe(store, 'firstStart', async () => { |
There was a problem hiding this comment.
seems like we could get rid of this logic by following the pattern of WalletAction's init function instead (maybe trying to restore from AppStorage instead).
or alternately, simplify WalletAction's init function by putting wallet initialization in this trigger..
There was a problem hiding this comment.
I thought about that too...
seems like we could get rid of this logic by following the pattern of WalletAction's init function instead (maybe trying to restore from AppStorage instead).
Problem is we can't use the settings action inside wallet init, because setting already depends on wallet i.e. it's injected to fetch the exchange rate.
or alternately, simplify WalletAction's init function by putting wallet initialization in this trigger..
Not super happy about that either because we can't really unit test this routine. Trying to keep all testable logic in actions.
The detection isn't based on IP address so VPN won't help. You'd need to change your |
|
Merging. Thanks @willpiers for the initial PR. Turns out this was quite a refactoring. @valentinewallace thanks for the review. |
@willpiers I took a look at the architecture and it turned out I had to refactor quite a bit to make this work. This probably wasn't a very good issue to get started with in the code base.
Closes #486