Add auto-update on hide workflow for Electron on Mac #1318

nbhargava · 2021-01-24T21:56:08Z

Details

This change allows the Electron app to auto-update when it has finished downloading the update and it is backgrounded.

Fixed Issues

Fixes #1011.

Tests

Install and run MinIO locally.
Point the publish spec in electron.config.js at the local server.
Publish a new version with the build number set very high (i.e. 1.0.1-999).
Build a new version for local development with a lower build number (i.e. 1.0.1-374). It is important that this is notarized, so you must use npm run desktop-build. This build should also have the publish spec pointed at MinIO.
Run the app.
Verify the app version is 1.0.1-374.
Once the new update has been downloaded, either hide or close the window.
The app may reboot a few times.
Foreground the app and verify that the app version is 1.0.1-999.

Tested On

Screenshots

Web

N/A

Mobile Web

N/A

Desktop

Video of the experience: https://www.dropbox.com/s/17jhevgcax2xz6x/Screen%20Recording%202021-02-24%20at%2010.13.15%20PM.mov?dl=0

Update modal:

iOS

N/A

Android

N/A

github-actions · 2021-01-24T21:56:22Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

nbhargava · 2021-01-24T21:57:12Z

I have read the CLA Document and I hereby sign the CLA

roryabraham

Haven't tested this yet, but just had a few comments to start. Overall looks very good!

Also, since this requires and external tool to test, I think it's probably worth including some documentation in the README explaining how to test the desktop auto-updater.

src/libs/checkForUpdates.js

desktop/main.js

nbhargava · 2021-01-29T00:56:28Z

Just pinging this again!

README.md

roryabraham · 2021-02-01T17:20:16Z

I ran into this issue notarizing the desktop app:

Package Summary:
 
1 package(s) were not uploaded because they had problems:
	/var/folders/0l/2cyzv7b14fx8rs5w8xpvvbyc0000gn/T/9185A388-AB1B-4C7A-9092-E8BF05A0CA7F/com.expensifyreactnative.chat.itmsp - Error Messages:
		The username rory@seahorsevineyards.com is not a member of the provider . Contact your team admin for assistance. (1296)
2021-02-01 09:16:22.785 altool[10536:83496] *** Error: Unable to notarize app.
2021-02-01 09:16:22.785 altool[10536:83496] *** Error: code 1296 (The username rory@seahorsevineyards.com is not a member of the provider . Contact your team admin for assistance. (1296))

Did you encounter this issue? Do you know what the problem is/how to resolve the error?

roryabraham

While I was not completely successful in testing this yet, I did have some suggestions for documentation improvements. I know it's not the best format (sorry), but hopefully you can apply the suggestions from this diff to your PR: autoUpdaterDocsImprovements.txt

nbhargava · 2021-02-02T03:21:30Z

I haven't seen that error before so I'm not 100% sure how to resolve it. My best guess is that you have local certificates installed that are for a member of some team/organization and that your personal account, rory@seahorsevineyards.com, is not allowed access to that. If that's the case, my guess would be that switching to your corporate account should let you upload properly.

This issue I found online seems roughly related: https://developer.apple.com/forums/thread/119445

nbhargava · 2021-02-02T03:27:32Z

^ real quick on the above: I only tested from a paid developer account, so I'm not sure if that's also a requirement.

roryabraham · 2021-02-02T18:03:21Z

Thanks for finding a way to test this and making those documentation updates, but it seems that the only way we can test this is to use a personal paid developer account (we don't want to use our production credentials). So could you please:

Move all the great documentation you have here to a separate README in the desktop directory (there should be no diff on the README in the root).
Make a note in the documentation that in order to test with a notarized build you'll need to have a paid apple developer account.
Post a screen recording of the auto-update happening so we can see that it works for you in real time and not just screenshots?

Thanks!

nbhargava · 2021-02-02T23:36:39Z

Updated the README as described and below is a video cast of the experience: https://www.dropbox.com/s/un6xgb4gepibmm6/Screen%20Recording%202021-02-02%20at%206.26.57%20PM.mov?dl=0

The flickering on the screen you see right before we re-load it is Big Sur's "verification" of the binary. It is sometimes slow and sometimes this fast.

roryabraham

Excellent work!

roryabraham · 2021-02-03T19:27:15Z

Hi @nbhargava, do you think it would be possible to have the auto-update happen in the middle of the night (even if the laptop is sleeping), rather than when the app is backgrounded?

nbhargava · 2021-02-04T21:30:36Z

Shouldn't be too difficult. What counts as middle of the night? 11pm-3am local time?

Also do you want it to be both middle of the night AND backgrounded or just middle of the night?

nbhargava · 2021-02-04T21:31:27Z

Oh sorry, missed the part about the laptop sleeping. If the laptop is sleeping I don't know how we'd be able to resume control to initiate the update. Not sure if that changes things.

roryabraham · 2021-02-05T00:30:52Z

Hi @nbhargava, after chatting about this internally a bit, we've decided on a change of plans.

Long-term, I think we're going to create a separate issue to investigate remote-hosting the JS bundle so that electron can auto-update its JS without having to close and reopen at all (like the Slack desktop client).

For this PR, could you please update it so that instead of closing and restarting the app when it's hidden, instead we just provide a custom notification to autoUpdater.checkForUpdatesAndNotify that includes an Update Now button. If the Update Now button in the notification is clicked, then it should close the app, install the update, and restart it (no need to immediately hide the app on relaunch).

Thanks!

nbhargava · 2021-02-06T16:01:14Z

I'll take a swing at this and see what I can do. The part that may be tricky is handling the fact that the "install" part of quitAndInstall doesn't seem to always fire reliably.

nbhargava · 2021-02-06T19:02:29Z

@roryabraham -- I've uploaded a new version with the requested changes. Here is a screen recording: https://www.dropbox.com/s/n8a04piq2nijhnv/Screen%20Recording%202021-02-06%20at%201.56.26%20PM.mov?dl=0

roryabraham · 2021-02-09T01:07:06Z

Hi @nbhargava, we'd really prefer if the notification and modal maintain a look-and-feel that's consistent with the rest of our application. So that means instead of using Electron's dialog.showMessageBoxSync:

Please use a browser notification to inform the user that there's an update available and to provide a button to trigger the update. We have a LocalNotification module which ideally could be extended to have a showUpdateNotification function.
Please use a react-native-modal component to display the confirmation dialog. We have a Modal module you can use to start.

The modules I linked above use the ES6 module system, so if you want to use them (which would be ideal), you may have to refactor our Electron configuration to enable the importation of ES6 modules into desktop/main.js. Do you think that's something you'd be up to? I noticed that the Electron v12 beta uses Node 14, and therefore should offer interoperability between CJS and ES6 modules without the use of an external package such as ESM. There is some more info in this GH issue.

roryabraham · 2021-02-09T01:26:15Z

Also, for the record @nbhargava has completed the task we initially gave him, and my previous comment constitutes a change in the scope-of-work. Do we have other examples for what we do when we increase the scope-of-work after the fact? cc @michaelhaxhiu @laurenreidexpensify @puneetlath – have you guys encountered this situation yet?

puneetlath · 2021-02-09T02:52:16Z

I think in that case let's just increase the scope of the Upwork job/pay by the same scale as the increase in scope.

michaelhaxhiu · 2021-02-09T11:59:33Z

+1 to that @puneetlath. Let's provide a bonus for the additional scope if it's agreeable for @nbhargava !

nbhargava · 2021-02-15T16:37:05Z

At the time, I was trying to import LocalNotification, but as you mention, it's much cleaner to just use Electron's IPC to get this done.

FWIW, in the future, the esm strategy does work quite well if you do need to end up importing ES6 modules in the future.

nbhargava · 2021-02-21T23:40:48Z

This new diff has the updates that pop open a modal when the download is available and then triggers the download when the user interacts with the popover. Below is a video of the interaction:

https://www.dropbox.com/s/6oa7p4qtnms58yr/Screen%20Recording%202021-02-21%20at%204.19.05%20PM.mov?dl=0

roryabraham · 2021-02-22T16:07:07Z

@nbhargava The video you've provided looks great to me! @AndrewGable, what do you think?

However, I noticed your code has several lint and test failures, so could you please work on resolving those before I dive into a thorough review?

nbhargava · 2021-02-22T16:08:38Z

Yup, I realized yesterday that there are some rebase issues due to refactors that happened while I was developing.

Will update later today and re-ping the thread once those are fixed.

AndrewGable · 2021-02-22T16:54:45Z

Looks great to me, cc @Expensify/design to make sure this is the UI element we'd like.

shawnborton · 2021-02-22T16:57:57Z

Are the screenshots on the original comment correct? From my understanding of reading through this, it seems like there should be some kind of Update button but I wasn't seeing one in the screenshots. It also looks like the video link you provided was deleted.

AndrewGable · 2021-02-22T16:59:42Z

I believe this is the latest video

shawnborton · 2021-02-22T17:08:33Z

Got it, @nbhargava can you update the original comment with both the video as well as static screenshots that clearly show the update flow?

In terms of the UI, I think it's great and is the best thing we can do right now before we have some kind of in-app notification/growl/toast component created. We can address that as a separate problem/solution when we start to develop the need for it.

nbhargava · 2021-02-24T03:59:52Z

Updated the original comment with the video & a static screenshot of the modal and fixed all the rebase issues

Not sure how to re-run the iOS tests, but from the log output: This is an error with npm itself, so I'm not too sure that I'm newly causing this error?

roryabraham

Overall code looks good, just a few minor changes. I re-ran the iOS tests and they passed, so you're good there.

Also, I noticed that the UpdateAppModal module has an index.native.js and index.website.js that are identical. We just fixed a webpack bug yesterday so that you shouldn't need to do that anymore. Just put your desktop-specific code in index.desktop.js and the default in index.js and it should work.

desktop/main.js

src/components/UpdateAppModal/BaseUpdateAppModal.js

src/libs/Notification/LocalNotification/BrowserNotifications.js

src/setup/index.desktop.js

src/components/UpdateAppModal/BaseUpdateAppModal.js

roryabraham · 2021-02-24T07:01:17Z

src/components/UpdateAppModal/BaseUpdateAppModal.js

+                            >
+                                Update App
+                            </Text>
+                        </TouchableOpacity>


NAB, but might be nice to have a "Maybe later" cancel button too.

Fix double-add Lint fixes Add more documentation Update docs Move README to desktop/ Show modal prompting user to restart Use modal to initiate download of update

nbhargava · 2021-02-25T03:17:02Z

Updated with your feedback and added a new video/screenshot to the top.

roryabraham · 2021-02-25T15:46:02Z

src/components/UpdateAppModal/BaseUpdateAppModal.js

+
+    // Version string for the app to update to.
+    // eslint-disable-next-line react/no-unused-prop-types
+    version: PropTypes.string,


Sorry I missed it in the last review, but if this prop is unused then we should just get rid of it.

Okay, actually I see why you did this. NAB but what I would've done is:

Create a new file called src/components/UpdateAppModal/UpdateAppModalPropTypes

Export the props types and default props you have in here as named exports like export {propTypes, defaultProps} from that file

In UpdateAppModal/index.js and UpdateAppModal/desktop.index.js, use the prop types from step 2, unaltered. Then just pass the one prop that's needed in BaseUpdateAppModal – onSumbit

In BaseUpdateAppModal, set the propTypes to _.omit(propTypes, 'version'). Same for default props.

roryabraham · 2021-02-25T15:57:12Z

src/components/UpdateAppModal/index.desktop.js

+import BaseUpdateAppModal from './BaseUpdateAppModal';
+
+const UpdateAppModal = (props) => {
+    const onSubmit = () => {


From our style guide...

When you have an event handler, do not prefix it with "on" or "handle". The method should be named for what it does, not what it handles.

So I would just rename this to updateApp

roryabraham · 2021-03-01T18:44:15Z

@nbhargava Any update here?

nbhargava · 2021-03-02T12:48:45Z

Sorry, forgot to push! The video is working for me, even in an incognito window. Are you still having issues with it?

roryabraham

LGTM and great work 👍

marcaaron · 2021-03-02T20:09:27Z

propTypes warnings were not fixed :( we should just make them errors

roryabraham · 2021-03-02T20:33:53Z

propTypes warnings were not fixed

Idk why I didn't think about this some more (should have held on merging), but it seems like a false warning to me. The prop types were imported from another file then altered, but should actually include onSubmit.

nbhargava requested a review from a team as a code owner January 24, 2021 21:56

botify requested review from timszot and removed request for a team January 24, 2021 21:56

roryabraham assigned nbhargava Jan 25, 2021

roryabraham requested changes Jan 25, 2021

View reviewed changes

src/libs/checkForUpdates.js Outdated Show resolved Hide resolved

desktop/main.js Outdated Show resolved Hide resolved

desktop/main.js Outdated Show resolved Hide resolved

desktop/main.js Outdated Show resolved Hide resolved

nbhargava requested a review from roryabraham January 26, 2021 02:30

roryabraham reviewed Feb 1, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

roryabraham requested changes Feb 1, 2021

View reviewed changes

nbhargava requested a review from roryabraham February 2, 2021 23:36

roryabraham previously approved these changes Feb 3, 2021

View reviewed changes

nbhargava dismissed roryabraham’s stale review via df9f6f5 February 6, 2021 19:00

nbhargava requested a review from roryabraham February 6, 2021 19:02

roryabraham mentioned this pull request Feb 17, 2021

Rename .jsx files to .js and vice versa Expensify/expensify-common#334

Closed

roryabraham requested changes Feb 24, 2021

View reviewed changes

roryabraham reviewed Feb 24, 2021

View reviewed changes

src/components/UpdateAppModal/BaseUpdateAppModal.js Outdated Show resolved Hide resolved

roryabraham reviewed Feb 24, 2021

View reviewed changes

nbhargava added 5 commits February 24, 2021 22:00

Add auto-update on hide workflow

b735e46

Fix double-add Lint fixes Add more documentation Update docs Move README to desktop/ Show modal prompting user to restart Use modal to initiate download of update

Remove electron-is-dev

e5f94fa

Fix package-lock

a0fd4cb

Fix rebase issues

0b2362d

Respond to reviewer feedback

99a612a

nbhargava requested a review from roryabraham February 25, 2021 03:16

roryabraham requested changes Feb 25, 2021

View reviewed changes

Update props

72b9003

nbhargava requested a review from roryabraham March 2, 2021 12:48

roryabraham approved these changes Mar 2, 2021

View reviewed changes

roryabraham merged commit ab1f1e4 into Expensify:master Mar 2, 2021

github-actions bot locked and limited conversation to collaborators Mar 2, 2021

Add auto-update on hide workflow for Electron on Mac #1318

Add auto-update on hide workflow for Electron on Mac #1318

Conversation

nbhargava commented Jan 24, 2021 • edited Loading

Details

Fixed Issues

Tests

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented Jan 24, 2021 • edited Loading

nbhargava commented Jan 24, 2021

roryabraham left a comment

Choose a reason for hiding this comment

nbhargava commented Jan 29, 2021

roryabraham commented Feb 1, 2021

roryabraham left a comment • edited Loading

Choose a reason for hiding this comment

nbhargava commented Feb 2, 2021

nbhargava commented Feb 2, 2021

roryabraham commented Feb 2, 2021

nbhargava commented Feb 2, 2021

roryabraham left a comment

Choose a reason for hiding this comment

roryabraham commented Feb 3, 2021

nbhargava commented Feb 4, 2021

nbhargava commented Feb 4, 2021

roryabraham commented Feb 5, 2021

nbhargava commented Feb 6, 2021

nbhargava commented Feb 6, 2021

roryabraham commented Feb 9, 2021 • edited Loading

roryabraham commented Feb 9, 2021

puneetlath commented Feb 9, 2021

michaelhaxhiu commented Feb 9, 2021

nbhargava commented Feb 15, 2021

nbhargava commented Feb 21, 2021

roryabraham commented Feb 22, 2021 • edited Loading

nbhargava commented Feb 22, 2021

AndrewGable commented Feb 22, 2021

shawnborton commented Feb 22, 2021

AndrewGable commented Feb 22, 2021

shawnborton commented Feb 22, 2021

nbhargava commented Feb 24, 2021

roryabraham left a comment • edited Loading

Choose a reason for hiding this comment

roryabraham Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

nbhargava commented Feb 25, 2021

roryabraham Feb 25, 2021

Choose a reason for hiding this comment

roryabraham Feb 25, 2021

Choose a reason for hiding this comment

roryabraham Feb 25, 2021

Choose a reason for hiding this comment

roryabraham commented Mar 1, 2021

nbhargava commented Mar 2, 2021

roryabraham left a comment

Choose a reason for hiding this comment

marcaaron commented Mar 2, 2021

roryabraham commented Mar 2, 2021

nbhargava commented Jan 24, 2021 •

edited

Loading

github-actions bot commented Jan 24, 2021 •

edited

Loading

roryabraham left a comment •

edited

Loading

roryabraham commented Feb 9, 2021 •

edited

Loading

roryabraham commented Feb 22, 2021 •

edited

Loading

roryabraham left a comment •

edited

Loading

roryabraham Feb 24, 2021 •

edited

Loading