Skip to content

[No QA] Fix desktop for developers without .env file#7719

Merged
mountiny merged 1 commit intomainfrom
Rory-FixElectronWithNoDotEnv
Feb 12, 2022
Merged

[No QA] Fix desktop for developers without .env file#7719
mountiny merged 1 commit intomainfrom
Rory-FixElectronWithNoDotEnv

Conversation

@roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Feb 12, 2022

cc @kidroca

Details

Following-up on #7693 (comment) – the Electron app is currently broken for developers not having a .env file.

Fixed Issues

$ n/a

Tests

  1. Comment out all lines on your .env or temporarily move it somewhere else.
  2. Run npm run desktop
  3. Verify that the app loads.
  • Verify that no new errors appear in the JS console

QA Steps

None (dev only)

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Desktop

image

@roryabraham roryabraham self-assigned this Feb 12, 2022
@roryabraham roryabraham marked this pull request as ready for review February 12, 2022 22:36
@roryabraham roryabraham requested a review from a team as a code owner February 12, 2022 22:36
@MelvinBot MelvinBot requested review from mountiny and removed request for a team February 12, 2022 22:36
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mountiny mountiny merged commit 8431ed4 into main Feb 12, 2022
@mountiny mountiny deleted the Rory-FixElectronWithNoDotEnv branch February 12, 2022 23:35
@OSBotify
Copy link
Contributor

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

Comment on lines 184 to +186
const devEnvConfig = dotenv.config({path: path.resolve(__dirname, '../.env')}).parsed;

if (devEnvConfig.USE_WEB_PROXY === 'true') {
if (devEnvConfig.USE_WEB_PROXY !== 'false') {
Copy link
Contributor

@kidroca kidroca Feb 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, sorry you still have to have an .env file to make this work
When you don't have an .env file parsed - devEnvConfig is undefined and you get this error:

[Main] 14:10:46.010 › (node:8157) UnhandledPromiseRejectionWarning: TypeError: Cannot read properties of undefined (reading 'USE_WEB_PROXY')
[Main]     at /Users/kidroca/Desktop/Freelance/Expensify.cash/desktop/main.js:186:34

What the proxy script does is a bit more flexible and you don't need to import path

dotenv.config();

if (process.env.USE_WEB_PROXY !== 'false') ...

When you don't pass a path to dotenv it will use .env by default
If there is such a file the variables defined would be assigned to process.env

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops ... I encountered this locally and had committed but not pushed the fix 😞

@OSBotify
Copy link
Contributor

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

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

@roryabraham roryabraham mentioned this pull request Feb 18, 2022
14 tasks
@OSBotify
Copy link
Contributor

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants