-
Notifications
You must be signed in to change notification settings - Fork 818
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
Update/replace remote #1102
Update/replace remote #1102
Conversation
|
||
const isEnvSet = 'ELECTRON_IS_DEV' in process.env; | ||
const getFromEnv = | ||
Number.parseInt(process.env.ELECTRON_IS_DEV!, 10) === 1; |
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.
I'm not sure we use ELECTRON_IS_DEV
, it might save a lot of time and space to just use the electron.app.isPackaged
method everywhere we need this. Less complexity too.
export const isFirstAppLaunch = (): boolean => { | ||
const checkFile = path.join( | ||
Electron.app.getPath('userData'), | ||
'.app-launched' |
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.
Has this been renamed from the electron-utils string? If it's been renamed, it will think everyone has opened the app for the first time regardless of if they have or not. We should maintain the old string.
@@ -0,0 +1,13 @@ | |||
import electron from 'electron'; | |||
|
|||
export const isDevelopment = () => { |
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.
Since this is duplicating code, why not use ipc to send this information from the main to renderer?
Nice work @cgarrovillo! I've left some comments based on the work I've done as well. I feel as though we may be working on exactly the same thing however, so I'm going to push my code tomorrow and link it here so we can sync up. |
Sounds good, feel free to overwrite my changes then, and we can talk about what's left. If there's tedious but easy tasks I can do I'll be happy to take those off your plate. |
Since #1096 is not moving forward, I think this pull request is a good step in the right direction. @cgarrovillo Would you be willing to address the pull request feedback? |
I've partially resolved this in #1148. Will continue after. Thanks for opening up a PR though ❤️ |
Before I move any further on this, thought I'd open this up for a review. This PR only removes
is
&isFirstAppLaunch
so far fromelectron-util
, tests showing no regression.One thing that stands out to me at first glance in the codebase is utils/ needing to be shared between main/ & renderer/