Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

reitowo
Copy link

@reitowo reitowo commented Jun 22, 2023

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

The plugin-local-electron currently cannot work with plugin-vite (or any other plugin) because it claims startLogic, which is unnecessary because it just merely set the ELECTRON_OVERRIDE_DIST_PATH env var.

This PR:

  1. Moved this logic to init function instead of startLogic
  2. Changed function locateElectronExecutable to detect this ELECTRON_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 under node_modules...)

Make local-electron compatible with vite plugin
@reitowo reitowo requested a review from a team as a code owner June 22, 2023 07:59
@reitowo reitowo changed the title Make plugin-local-electron compatible with Windows fix: Make plugin-local-electron compatible with Windows Jun 22, 2023
@reitowo reitowo changed the title fix: Make plugin-local-electron compatible with Windows fix(plugin-local-electron): Make the plugin compatible with Windows Jun 23, 2023
@reitowo reitowo changed the title fix(plugin-local-electron): Make the plugin compatible with Windows fix(plugin-local-electron): Make the plugin compatible on Windows Jun 23, 2023
@reitowo reitowo changed the title fix(plugin-local-electron): Make the plugin compatible on Windows fix(plugin-local-electron): Make the plugin compatible on Windows, and can live with other plugins Jun 23, 2023
@reitowo
Copy link
Author

reitowo commented Jun 24, 2023

@caoxiemeihao Can I bother you review this?

@reitowo
Copy link
Author

reitowo commented Jun 28, 2023

@VerteDinde Can you review this PR?😘

Comment on lines +16 to +22
init = (): void => {
if (this.enabled) {
this.checkPlatform(process.platform);
process.env.ELECTRON_OVERRIDE_DIST_PATH = this.config.electronPath;
}
};

Copy link
Member

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?

Copy link
Author

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.

Copy link
Member

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

Comment on lines +49 to +50
after(() => {
delete process.env.ELECTRON_OVERRIDE_DIST_PATH;
Copy link
Member

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:

afterEach(() => {
delete process.env.ELECTRON_OVERRIDE_DIST_PATH;
});

Copy link
Author

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.

Copy link
Member

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?

@BlackHole1 BlackHole1 requested a review from a team June 28, 2023 08:19
@erickzhao erickzhao self-requested a review August 18, 2023 18:27
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.

None yet

2 participants