-
Notifications
You must be signed in to change notification settings - Fork 414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Auto Update with electron-updater (WIP) #808
Conversation
732b2bf
to
991e416
Compare
2980d96
to
ec27320
Compare
src/main/main.js
Outdated
|
||
autoUpdater.logger = log; | ||
autoUpdater.on('checking-for-update', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make sense to split the update logic into it's own file if it's not interacting with main.js
much. main.js
is already a little big and cluttered.
0daa2b3
to
d31ed05
Compare
5e0615a
to
94dde48
Compare
02d1417
to
edefecd
Compare
49edd5e
to
5c5f14c
Compare
65b4d93
to
483809b
Compare
# Workaround: TeamCity expects the dmg to be in dist/mac, but in the new electron-builder | ||
# it's put directly in dist/ (the right way to solve this is to update the TeamCity config) | ||
if $OSX; then | ||
cp dist/*.dmg dist/mac |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be better to change TeamCity config directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would, but I figured we would worry about it later. It's already going to be a little complicated to roll out auto update.
package.json
Outdated
@@ -20,7 +20,6 @@ | |||
"dev": "electron-webpack dev", | |||
"compile": "electron-webpack && yarn extract-langs", | |||
"build": "yarn compile && electron-builder build", | |||
"build:dir": "yarn build -- --dir -c.compression=store -c.mac.identity=null", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep this. It was requested here #933.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a merge error; will fix.
src/main/index.js
Outdated
// Keeps track of whether the user has accepted an auto-update through the interface. | ||
let autoUpdateAccepted = false; | ||
|
||
// This is used to keep track of whether we are showing he special dialog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: he
=> the
.
src/main/index.js
Outdated
@@ -68,7 +86,26 @@ app.on('activate', () => { | |||
if (!rendererWindow) rendererWindow = createWindow(); | |||
}); | |||
|
|||
app.on('will-quit', () => { | |||
app.on('will-quit', (e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use a more descriptive name (http://airbnb.io/javascript/#naming--descriptive)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this apply even to e
? That has a standard meaning of "event," similar to x
, y
, i
, etc.
If we want to be even more specific than "event," it would have to be willQuitEvent
or something, which sounds redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think event
would fit best here. We don't have multiple events in this callback function so no need to specify precisely the event, and it would also be redundant as you pointed it out. Anyway, this is a nit.
src/main/index.js
Outdated
import Daemon from './Daemon'; | ||
import Tray from './Tray'; | ||
import createWindow from './createWindow'; | ||
|
||
// For now, log info messages in production for easier debugging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will electron-log
be removed before merge?
6fa470f
to
026adb7
Compare
We might need this later, but right now no logging is happening here.
e2f4ec9
to
572c56f
Compare
3f85556
to
e98231f
Compare
Build is fixed now and the UI is practically done. The main issue is that I'm getting bug at the very end of the process: when I call
appUpdater.quitAndRestart()
to trigger the install, it closes the window but doesn't exit the app or do the install. Hopefully it won't be too hard to figure out.