-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
autoUpdater.emit('update-not-available'); | ||
} else { | ||
// If no updates found check for updates every hour | ||
await B.delay(60 * 1000); |
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.
is the interval in seconds or millis?
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.
why don't we use setTimeout?
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.
ms; setTimeout does not return a promise and can't be awaited
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 changed it to 60 * 60 * 1000
so that it checks for updates every hour and not every minute.
const res = await request.get(getFeedUrl(currentVersion)); | ||
if (res) { | ||
return JSON.parse(res); | ||
} else { |
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.
else is redundant
import { getFeedUrl } from './config'; | ||
|
||
export async function getUpdate (currentVersion) { | ||
const res = await request.get(getFeedUrl(currentVersion)); |
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 assume this may throw an exception in case of request failure
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.
Yep. Good catch. It should return false on error.
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.
love it
app/main/appium.js
Outdated
import AppiumMethodHandler from './appium-method-handler'; | ||
import request from 'request-promise'; | ||
import { checkNewUpdates } from './auto-updater/auto-updater'; |
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.
if you rename it to index.js
you can just do from './auto-updater'
;
const update = await getUpdate(app.getVersion()); | ||
if (update) { | ||
let {name, notes, pub_date} = update; | ||
pub_date = moment(pub_date).format('MMM Do YYYY, h:mma'); |
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.
even if update
really has to have snake_case, we should name our own variables using camelCase
@@ -0,0 +1,8 @@ | |||
const config = { | |||
baseFeedUrl: `https://hazel-server-pxufsrwofl.now.sh`, |
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.
is this a stable url? should we pay for the ability to run it on say update.appium.io?
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.
@jlipps It's hosted by https://zeit.co/now... not sure how stable that is. I ran this server by following the instructions on zeit/hazel git page.
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.
The URL itself is permanent and I'm pretty sure the server should be up-and-running permanently; if it ever were taken down, I think we would get notice first.
|
||
before(async function () { | ||
const latestReleaseUrl = `https://api.github.com/repos/appium/appium-desktop/releases/latest`; | ||
const res = JSON.parse(await request.get(latestReleaseUrl, { headers: {'user-agent': 'node.js'} })); |
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.
{headers: {'user-agent': 'node.js'}}
f70ec43
to
df679fc
Compare
* Native autoUpdater downloads updates automatically * We want to be able to check if updates are available first * So, make a call to /update endpoint directly and pass in the version to check if there's an update available
* Use the native electron dialogs instead of using a custom React workflow * Right now it automatically downloads it * Next step will be to check for availability first before proceeding to download
df04fae
to
5167875
Compare
@jlipps Is this good to merge? |
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 one question, otherwise LGTM
@@ -1,5 +1,5 @@ | |||
environment: | |||
nodejs_version: "6" | |||
nodejs_version: "7" |
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.
why 7 and not 8?
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.
8 caused the issues with WebDriverAgent privateheaders
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.
ok
autoupdate.checkForUpdates()
and wait for download(also related to https://github.com/appium/appium-desktop/issues/266)