[No QA] Simplify electron-builder configs by roryabraham · Pull Request #7684 · Expensify/App

roryabraham · 2022-02-10T21:02:05Z

Details

Following up on #7665 to do some cleanup.

Fixed Issues

n/a

Tests

Run npm run desktop, and verify that the app loads correctly.
Make a visible change to a React Native file, then save. The desktop app should hot-reload.
Run npm run desktop-build-local, install the app and make sure it works! It should have a production app icon, not a staging app icon.
Run npm run desktop-build-staging-local, install the app and make sure it works! It should have a staging app icon, not a production app icon.
Merge this PR and pray that it doesn't break our desktop deploys 🙃

Verify that no errors appear in the JS console

QA Steps

None.

Verify that no errors appear in the JS console

Tested On

Screenshots

Desktop (production build)

Desktop (staging build)

…ing builds easier

…ironment variable from GitHub Actions

botify · 2022-02-10T21:02:06Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

botify · 2022-02-11T17:13:57Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

botify · 2022-02-11T17:19:05Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

botify · 2022-02-11T17:19:18Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

neil-marcellini · 2022-02-11T18:02:15Z

I'm going to un-assign from this review because I've been swamped by PRs lately and this one is quite a bit over my head.

tgolen

Looking really great! A few suggestions

config/electronBuilder/electronBuilder.ghactions.config.js

config/electronBuilder/electronBuilder.local.config.js

package.json

botify · 2022-02-11T18:22:22Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

roryabraham · 2022-02-11T18:26:16Z

Updated!

botify · 2022-02-11T18:27:35Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

…ages

botify · 2022-02-11T19:57:13Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

roryabraham · 2022-02-11T19:58:49Z

Taking this off draft and marking it as ready for review, but I think we should keep it on hold until it's not DBSF.

tgolen

Cool, this looks good to me. How come it's on HOLD?

tgolen · 2022-02-14T15:46:26Z

Oh, because it was Friday, got it. You can self merge this when you are ready :D

botify · 2022-02-14T17:08:08Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

OSBotify · 2022-02-14T17:19:52Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

kidroca

I'm working on reducing desktop size here: #7744 and noticed some merge conflicts.
I decided to leave some notes

Do we expect any more Desktop changes, because I'm having to change my code each time a change is made?

(managed to reduce the installer to ~90Mb)

kidroca · 2022-02-14T17:40:34Z

config/electronBuilder/electronBuilder.ghactions.config.js

+module.exports = {
+    ...baseElectronBuilderConfig,
+    publish: [{
+        provider: 's3',
+        bucket: process.env.ELECTRON_ENV === 'staging' ? 'staging-expensify-cash' : 'expensify-cash',
+        channel: 'latest',
+    }],
+    afterSign: './desktop/notarize.js',


Were there any problems when this was part of the single config?
I have no problems building local builds without this change?

E.g. I don't use --publish and this just creates a local build for me, no need for desktop-build-local and desktop-build-local-staging scripts

Separate question: If notarize.js is something only used for config/build signing why keep it in desktop ? it gets packed to the electron app due to the desktop/*.js pattern

Were there any problems when this was part of the single config?

I always run into issues with the notarizing step. You don't experience that?

Separate question: If notarize.js is something only used for config/build signing why keep it in desktop ?

Just because it's specific to the desktop application.

kidroca · 2022-02-14T17:48:51Z

package.json

+    "desktop": "export ELECTRON_ENV=development && node desktop/start.js",
+    "desktop-build": "export ELECTRON_ENV=production && webpack --config config/webpack/webpack.prod.js --platform desktop && electron-builder --config config/electronBuilder/electronBuilder.ghactions.config.js",
+    "desktop-build-staging": "export ELECTRON_ENV=staging && webpack --config config/webpack/webpack.staging.js --platform desktop && electron-builder --config config/electronBuilder/electronBuilder.ghactions.config.js",
+    "desktop-build-local": "export ELECTRON_ENV=production && webpack --config config/webpack/webpack.prod.js --platform desktop && electron-builder --config config/electronBuilder/electronBuilder.local.config.js",
+    "desktop-build-staging-local": "export ELECTRON_ENV=staging && webpack --config config/webpack/webpack.staging.js --platform desktop && electron-builder --config config/electronBuilder/electronBuilder.local.config.js",


This looks a bit odd - none of the other build scripts web/mobile have to set ENV vars in their package script.
It might be better to extract a script like desktop/start.js but perhaps desktop/build.js where the appropriate dotenv file can be loaded. The least it would hide these details and make the scripts shorter

roryabraham · 2022-02-14T19:22:22Z

Do we expect any more Desktop changes

I don't expect any more for now.

OSBotify · 2022-02-15T19:32:07Z

🚀 Deployed to staging by @roryabraham in version: 1.1.39-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-02-23T21:21:26Z

🚀 Deployed to production by @timszot in version: 1.1.39-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

roryabraham added 6 commits February 10, 2022 12:26

Move afterSign to electron.config.js

27afd80

Split apart local build and publishing pieces to make local prod/stag…

d126a85

…ing builds easier

Pass NODE_ENV explicitly in package.json instead of relying on an env…

4c1b388

…ironment variable from GitHub Actions

Delete now-unused electron.config.js

87837c0

Fix import and electronBuilder configs?

a9a9666

Update documentation

63c282e

roryabraham self-assigned this Feb 10, 2022

roryabraham added 4 commits February 11, 2022 08:52

Add NODE_ENV=production to package.json scripts

ca73ad8

Rename electronBuilder configs

3419249

Move NODE_ENV dev to package.json

f880d0a

Rename environment constant to standardize on the longer form

6ae2075

roryabraham changed the title ~~Simplify electron-builder configs~~ [HOLD] Simplify electron-builder configs Feb 11, 2022

roryabraham requested review from neil-marcellini and tgolen February 11, 2022 17:15

roryabraham changed the title ~~[HOLD] Simplify electron-builder configs~~ [HOLD][No QA] Simplify electron-builder configs Feb 11, 2022

This comment was marked as off-topic.

Sign in to view

Merge branch 'main' into Rory-LessConfusingDesktopEnvironments

8978c94

This comment was marked as resolved.

Sign in to view

roryabraham added 2 commits February 11, 2022 09:56

Update dotenv environment to match long form

61d5361

Fix environment badge

0d3aaf6

neil-marcellini removed their request for review February 11, 2022 18:02

tgolen requested changes Feb 11, 2022

View reviewed changes

config/electronBuilder/electronBuilder.ghactions.config.js Outdated Show resolved Hide resolved

config/electronBuilder/electronBuilder.local.config.js Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

roryabraham added 2 commits February 11, 2022 10:24

Move desktop script to be with other desktop scripts

481bb3d

Cleanup local variables in electronBuilder configs

86a8fbd

Rename NODE_ENV to ELECTRON_ENV to prevent problems in 3rd party pack…

890bdf1

…ages

roryabraham marked this pull request as ready for review February 11, 2022 19:58

roryabraham requested a review from a team as a code owner February 11, 2022 19:58

roryabraham requested a review from tgolen February 11, 2022 19:58

MelvinBot requested review from Gonals and removed request for a team February 11, 2022 19:58

tgolen approved these changes Feb 14, 2022

View reviewed changes

roryabraham changed the title ~~[HOLD][No QA] Simplify electron-builder configs~~ [No QA] Simplify electron-builder configs Feb 14, 2022

roryabraham merged commit 24b9474 into main Feb 14, 2022

roryabraham deleted the Rory-LessConfusingDesktopEnvironments branch February 14, 2022 17:08

kidroca reviewed Feb 14, 2022

View reviewed changes

OSBotify mentioned this pull request Feb 15, 2022

Deploy Checklist: New Expensify 2022-02-15 #7759

Closed

85 tasks

Conversation

roryabraham commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Desktop (production build)

Desktop (staging build)

Uh oh!

botify commented Feb 10, 2022

Uh oh!

botify commented Feb 11, 2022

Uh oh!

botify commented Feb 11, 2022

Uh oh!

botify commented Feb 11, 2022

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as resolved.

This comment was marked as resolved.

neil-marcellini commented Feb 11, 2022

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

botify commented Feb 11, 2022

Uh oh!

roryabraham commented Feb 11, 2022

Uh oh!

botify commented Feb 11, 2022

Uh oh!

botify commented Feb 11, 2022

Uh oh!

roryabraham commented Feb 11, 2022

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

tgolen commented Feb 14, 2022

Uh oh!

botify commented Feb 14, 2022

Uh oh!

OSBotify commented Feb 14, 2022

Uh oh!

kidroca left a comment

Choose a reason for hiding this comment

Uh oh!

kidroca Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

roryabraham Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

kidroca Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

roryabraham commented Feb 14, 2022

Uh oh!

OSBotify commented Feb 15, 2022

Uh oh!

OSBotify commented Feb 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

roryabraham commented Feb 10, 2022 •

edited

Loading