-
-
Notifications
You must be signed in to change notification settings - Fork 520
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
fix(plugin-local-electron): Make the plugin compatible on Windows, and can live with other plugins #3259
base: main
Are you sure you want to change the base?
Conversation
Make local-electron compatible with vite plugin
@caoxiemeihao Can I bother you review this? |
@VerteDinde Can you review this PR?😘 |
init = (): void => { | ||
if (this.enabled) { | ||
this.checkPlatform(process.platform); | ||
process.env.ELECTRON_OVERRIDE_DIST_PATH = this.config.electronPath; | ||
} | ||
}; | ||
|
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.
The changes here seem unnecessary because init
and startLogic
are responsible for different things. init is responsible for initializing the template, while startLogic
is responsible for starting Electron. So I didn't quite understand the modifications here. Could you please explain in detail the necessity of these changes?
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.
This plugin merely just set the environment variable, and using startLogic
will prevent it from using with other plugins like vite
.
This could go to constructor if init
is not a right place to go.
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 very familiar with this piece of code. Perhaps other people might have some suggestions.
@caoxiemeihao @electron/forgers PLAT
after(() => { | ||
delete process.env.ELECTRON_OVERRIDE_DIST_PATH; |
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.
This is duplicated with the existing code. See:
forge/packages/plugin/local-electron/test/LocalElectronPlugin_spec.ts
Lines 16 to 18 in a62c2a1
afterEach(() => { | |
delete process.env.ELECTRON_OVERRIDE_DIST_PATH; | |
}); |
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.
It is not, because the delete you referenced is in the other scope.
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.
Ah, my bad for not catching that! Would it be better to use afterEach
?
@reitowo please can you resolve conflicts? |
Summarize your changes:
The
plugin-local-electron
currently cannot work withplugin-vite
(or any other plugin) because it claimsstartLogic
, which is unnecessary because it just merely set theELECTRON_OVERRIDE_DIST_PATH
env var.This PR:
init
function instead ofstartLogic
locateElectronExecutable
to detect thisELECTRON_OVERRIDE_DIST_PATH
env var to make the project directly starts the binary in the wanted path. (Because on Windows this env variable makes no effect and still launch the binary undernode_modules
...)