Auto Update with electron-updater (WIP) #808

2017-12-04T08:44:31+01:00

alexliebowitz commented

2017-12-04 08:44:31 +01:00

(Migrated from github.com)

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.

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.

IGassmann (Migrated from github.com) approved these changes 2017-12-04 13:31:12 +01:00

kauffj (Migrated from github.com) reviewed 2017-12-04 16:24:55 +01:00

src/main/main.js

kauffj (Migrated from github.com) commented

2017-12-04 16:24:55 +01:00

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.

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.

kauffj (Migrated from github.com) requested changes 2018-01-09 00:22:24 +01:00

kauffj (Migrated from github.com) left a comment

Mostly copy

src/main/index.js

kauffj (Migrated from github.com) commented

2018-01-09 00:07:55 +01:00

"LBRY Will Upgrade"

kauffj (Migrated from github.com) commented

2018-01-09 00:08:51 +01:00

"LBRY has a pending upgrade. Please select "Yes" to install it on the prompt shown after this one."

(Assuming this is correct, and that prompt isn't shown until I ACK this one).

"LBRY has a pending upgrade. Please select "Yes" to install it on the prompt shown after this one." (Assuming this is correct, and that prompt isn't shown until I ACK this one).

src/renderer/modal/modalAutoUpdateDownloaded/view.jsx

kauffj (Migrated from github.com) commented

2018-01-09 00:14:26 +01:00

This isn't necessarily a blocker, but is there a strong reason we have to expose ipcRenderer inside of a component, as well as put all of the handling of these events inside of main/index.js? Could this component instead dispatch an APP_QUIT action, and a reducer could close the app? (Or the top-level component?)

This isn't necessarily a blocker, but is there a strong reason we have to expose `ipcRenderer` inside of a component, as well as put all of the handling of these events inside of `main/index.js`? Could this component instead dispatch an `APP_QUIT` action, and a reducer could close the app? (Or the top-level component?)

kauffj (Migrated from github.com) commented

2018-01-09 00:17:24 +01:00

"Use It Now"

kauffj (Migrated from github.com) commented

2018-01-09 00:20:18 +01:00

A new version of LBRY has been released, downloaded, and is ready for you to use pending a restart.

kauffj (Migrated from github.com) commented

2018-01-09 00:22:04 +01:00

"Not Now" or "Upgrade on Restart"

src/renderer/modal/modalAutoUpdateDownloaded/view.jsx

					
				@ -0,0 +26,4 @@

				        }}

				      >

				        <section>

				          <h3 className="text-center">{__("LBRY Leveled Up")}</h3>

kauffj (Migrated from github.com) commented

2018-01-09 00:16:01 +01:00

I noticed a <br/> was dropped here.

I noticed a `<br/>` was dropped here.

alexliebowitz (Migrated from github.com) reviewed 2018-01-09 01:09:43 +01:00

src/renderer/modal/modalAutoUpdateDownloaded/view.jsx

					
				@ -0,0 +26,4 @@

				        }}

				      >

				        <section>

				          <h3 className="text-center">{__("LBRY Leveled Up")}</h3>

alexliebowitz (Migrated from github.com) commented

2018-01-09 01:09:43 +01:00

There are other modals (most?) with no <br />, and it's not semantic, and seemed to look fine this way. What did you think of the look?

There are other modals (most?) with no `<br />`, and it's not semantic, and seemed to look fine this way. What did you think of the look?

alexliebowitz (Migrated from github.com) reviewed 2018-01-09 01:10:21 +01:00

src/main/index.js

alexliebowitz (Migrated from github.com) commented

2018-01-09 01:10:21 +01:00

You are correct, and I'll update the wording (along with all of the other wording changes you mentioned)

alexliebowitz (Migrated from github.com) reviewed 2018-01-09 01:27:36 +01:00

src/renderer/modal/modalAutoUpdateDownloaded/view.jsx

alexliebowitz (Migrated from github.com) commented

2018-01-09 01:27:36 +01:00

I was going back and forth on this. Technically, neither of these IPC calls have anything to do with the store, and they do not lead to DOM updates. The "autoUpdateAccepted" IPC call tells the main process that it's time to shut down and restart (not UI state) and the "autoUpdateDeclined" IPC call just tells the main process to remember that the update was declined so that it can show the extra dialog on Windows (again, not UI state; the dialog is created by the main process and only appears after the app window is closed).

Is it typical to put code in Redux-land when it is only used for its side effects and doesn't affect anything going on in the browser window?

I was going back and forth on this. Technically, neither of these IPC calls have anything to do with the store, and they do not lead to DOM updates. The "autoUpdateAccepted" IPC call tells the main process that it's time to shut down and restart (not UI state) and the "autoUpdateDeclined" IPC call just tells the main process to remember that the update was declined so that it can show the extra dialog on Windows (again, not UI state; the dialog is created by the main process and only appears after the app window is closed). Is it typical to put code in Redux-land when it is only used for its side effects and doesn't affect anything going on in the browser window?

kauffj (Migrated from github.com) reviewed 2018-01-09 21:36:13 +01:00

src/renderer/modal/modalAutoUpdateDownloaded/view.jsx

kauffj (Migrated from github.com) commented

2018-01-09 21:36:13 +01:00

I think this is fine to leave as-is, at least for now.

IGassmann (Migrated from github.com) requested changes 2018-01-23 20:02:38 +01:00

build/build.sh

					
				@ -82,0 +82,4 @@

				  # 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

IGassmann (Migrated from github.com) commented

2018-01-23 19:35:36 +01:00

Wouldn't be better to change TeamCity config directly?

package.json

IGassmann (Migrated from github.com) commented

2018-01-23 19:38:40 +01:00

We should keep this. It was requested here https://github.com/lbryio/lbry-app/pull/933.

src/main/index.js

IGassmann (Migrated from github.com) commented

2018-01-23 19:52:52 +01:00

nit: he => the.

nit: `he` => `the`.

IGassmann (Migrated from github.com) commented

2018-01-23 20:01:29 +01:00

Will electron-log be removed before merge?

Will `electron-log` be removed before merge?

src/main/index.js

					
				@ -68,7 +83,26 @@ app.on('activate', () => {

				  if (!rendererWindow) rendererWindow = createWindow();

				});

IGassmann (Migrated from github.com) commented

2018-01-23 19:57:04 +01:00

nit: use a more descriptive name (http://airbnb.io/javascript/#naming--descriptive)

alexliebowitz (Migrated from github.com) reviewed 2018-01-23 20:51:25 +01:00

src/main/index.js

					
				@ -68,7 +83,26 @@ app.on('activate', () => {

				  if (!rendererWindow) rendererWindow = createWindow();

				});

alexliebowitz (Migrated from github.com) commented

2018-01-23 20:51:25 +01:00

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

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

alexliebowitz (Migrated from github.com) reviewed 2018-01-23 20:51:50 +01:00

package.json

alexliebowitz (Migrated from github.com) commented

2018-01-23 20:51:50 +01:00

Just a merge error; will fix.

alexliebowitz (Migrated from github.com) reviewed 2018-01-23 20:52:21 +01:00

build/build.sh

					
				@ -82,0 +82,4 @@

				  # 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

alexliebowitz (Migrated from github.com) commented

2018-01-23 20:52:21 +01:00

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.

IGassmann (Migrated from github.com) reviewed 2018-01-23 21:11:00 +01:00

src/main/index.js

					
				@ -68,7 +83,26 @@ app.on('activate', () => {

				  if (!rendererWindow) rendererWindow = createWindow();

				});

IGassmann (Migrated from github.com) commented

2018-01-23 21:11:00 +01:00

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.

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.

Sign in to join this conversation.

No reviewers

kauffj

IGassmann