-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add electron auto updating #359
Conversation
…veChat into andrew-auto-update
…veChat into andrew-auto-update
…veChat into andrew-auto-update
…veChat into andrew-auto-update
…veChat into andrew-auto-update
…veChat into andrew-auto-update
…veChat into andrew-auto-update
…veChat into andrew-auto-update
…veChat into andrew-auto-update
|
||
on: | ||
push: | ||
branches: [master] |
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.
So this is a slight tweak on our deploy process.
Before
- Push to master kicks off deploy actions for web, mobile, and android
After
- Generate a new version on every push to master
- Push a new tag with that version
- All the deploy scripts will start when a new tag is pushed
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.
So in the future when we're using react-native for our full app, would we basically just change this to production
?
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.
Good question, I don't think we've really discussed how we will merge the deploy processes of the two repos quite yet.. The deploy process for ReactNative web app is quite simple.. Just throw it on an s3 bucket, refresh cloudflare cache and we are good. The deploy process for our existing front end is a bit more complicated.. I think that will be a larger discussion that probably warrants a design doc
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.
Sounds good 👍
@@ -138,8 +138,8 @@ android { | |||
applicationId "com.expensifyreactnative.chat" | |||
minSdkVersion rootProject.ext.minSdkVersion | |||
targetSdkVersion rootProject.ext.targetSdkVersion | |||
versionCode 5 | |||
versionName "1.0.1-4" | |||
versionCode 18 |
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.
Bugs me that these are off, once this is merged I will try to align them
const loadURL = serve({directory: 'dist'}); | ||
|
||
await app.whenReady(); |
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.
Removed awaits in this file, I think everything works but let me know if you spot any bugs.
Going to push a few more commits here to clean things up even further. |
Ok ready for a review! |
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.
LGTM!
return app.whenReady() | ||
.then(() => { | ||
const browserWindow = new BrowserWindow({ | ||
backgroundColor: '#FAFAFA', |
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.
NAB: You should use the colors object from the global stylesheet (I actually really think that colors deserve their own file under styles/, but that's beside the point)
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.
Good point, I will add that in a future PR.
@@ -6,7 +6,7 @@ const AppLinks = () => ( | |||
<> | |||
<Anchor | |||
style={[styles.sidebarFooterLink, styles.mr2]} | |||
href="https://chat.expensify.com/Chat.app.zip" | |||
href="https://chat.expensify.com/Chat.dmg" |
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.
Would this work on non-mac platforms?
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.
No, we need to compile the app for each specific platform (linux and windows is currently not supported)
Fixes #255
Adds an auto update check when you open the app, we can get more fancy with when we check if we'd like, but this is using default settings for the POC.
You will now see this notification when launching the app if there is a new update deployed (new updates are deployed on each commit)