[New Feature] Using node-keytar to store github token in safe place #321
Conversation
|
Thanks. Will have a look this weekend. |
| ```bash | ||
| $ npm run electron-rebuild | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Can we avoid treating Linux as a special case?
There was a problem hiding this comment.
OSes use different things to store secret so node-keytar (a native node module) have to deal with this issue.
From what I know, we have to rebuild native node modules b/c some pre-build libs won't work.
There was a problem hiding this comment.
Thanks for explaining:) I'm hesitant to add extra build steps for particular platforms, especially introducing external dependency by installing prerequisites. This often (significantly) increases the maintenance cost by making it hard to dig out or reproduce bugs, especially when the bug is related to the launch step. Maybe this module is not ready for integration at this moment.
There was a problem hiding this comment.
Yah, you're right. Using this module can increase failure rate and it's not safe as we want (atom/node-keytar#88)
app/index.js
Outdated
| const cachedImage = electronLocalStorage.get('image') | ||
| logger.debug(`-----> [${cachedImage.status}] cachedImage is ${cachedImage.data}`) | ||
|
|
||
| if (cachedProfile.status && cachedImage.status) { |
There was a problem hiding this comment.
Only the profile cache and token cache are required for launching. The image cache is optional. Given there is a relatively higher failure rate for image caching, this check will propagate this higher failure rate to the app launching.
There was a problem hiding this comment.
Yah, I pushed a commit to remove cachedImage.status
| updateDashboardModalStatus = { updateDashboardModalStatus } | ||
| updateActiveGistAfterClicked = { updateActiveGistAfterClicked } /> | ||
| </Provider>, | ||
| <Async |
There was a problem hiding this comment.
Sorry, I'm not good at JS so I don't get it. Could you teach me why using regular Promise is better ? Thanks !
Try to resolve #319