frontend: improve routing and introduce device switch #145
Conversation
benma
force-pushed
the
cleanroutes
branch
2 times, most recently
from
6f39642
to
c8f9176
Compare
| } | ||
| } | ||
| time.Sleep(time.Second) | ||
| } | ||
| } | ||
|
|
||
| // Start listens for inserted/removed devices forever. Run this in a goroutine. |
There was a problem hiding this comment.
Run this in a goroutine.
Why? 🤔
There was a problem hiding this comment.
Forgot to adjust the comment (was the same comment before refactor). Thanks! #150
| }); | ||
| if (!accountsInitialized) { | ||
| console.log('app.jsx route /'); // eslint-disable-line no-console | ||
| route('/', true); |
There was a problem hiding this comment.
Is it this why my accounts are no longer directly activated when I use the software-based keystore? 🤔
There was a problem hiding this comment.
No, that is because they are activated in device.jsx after unlock. Similar should be done in the software based one, or move this to a more generic place.
Btw. this particular lines I restored later, but adjusted to only apply when you are in /accounts/ (otherwise it would override othe redirects, e.g. to the inserted device, in a race).
| path="/device/:deviceID" | ||
| deviceIDs={deviceIDs} /> | ||
| <Device | ||
| key={devices} |
There was a problem hiding this comment.
Why do we need a key property here? 🤔 And this might no longer be valid/possible after preactjs/preact#1248.
There was a problem hiding this comment.
I picked apart multiple commits and this belonged to a different one afaik. Anyway, it carries over here:
Needed to that the component remounts with different keys, to trigger the componentDidMount().
There was a problem hiding this comment.
Well, that sounds like a misuse of componentDidMount then in https://github.com/digitalbitbox/bitbox-wallet-app/blob/97bd934996a3d5ba027b6ec782d151fe4b445ec6/frontends/web/src/routes/device/deviceswitch.tsx#L30
Maybe use componentWillReceiveProps instead? (https://github.com/developit/preact#components).
There was a problem hiding this comment.
Or at least add a comment why the key properties are needed there. This is very subtle and uncommon (as far as I can judge) and it's shortcuts like this that make our code so fragile.
There was a problem hiding this comment.
Can do. After doing the research on resetting components, using the key seems to be a normal solution, but noted.
Didn't use componentWillReceiveProps because it's use seems discouraged:
https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops
There was a problem hiding this comment.
You can also use https://reactjs.org/docs/react-component.html#componentdidupdate, if you prefer. And I don't know to what degree Preact will follow such deprecations.
No description provided.