Skip to content

fix(plugin-local-electron): Make the plugin compatible on Windows, and can live with other plugins #3259

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.

Sign up for GitHub

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

Closed
reitowo wants to merge 4 commits into from
Closed

fix(plugin-local-electron): Make the plugin compatible on Windows, and can live with other plugins #3259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e38c82b
Make local-electron works better under Windows
reitowo Jun 22, 2023
24d54e9
Unit tests
reitowo Jun 22, 2023
0f4e105
Update test case for local-electron plugin
reitowo Jun 22, 2023
4af4a7f
Unit tests
reitowo Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/api/core/src/util/electron-executable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ export function pluginCompileExists(packageJSON: PackageJSON): boolean {
}

export default async function locateElectronExecutable(dir: string, packageJSON: PackageJSON): Promise<string> {
const overrideDist = process.env.ELECTRON_OVERRIDE_DIST_PATH;
if (overrideDist) {
console.info('Using electron dist from ELECTRON_OVERRIDE_DIST_PATH: ' + overrideDist);
return overrideDist;
}

let electronModulePath: string | undefined = await getElectronModulePath(dir, packageJSON);
if (electronModulePath?.endsWith('electron-prebuilt-compile') && !pluginCompileExists(packageJSON)) {
console.warn(
Expand Down
16 changes: 7 additions & 9 deletions packages/plugin/local-electron/src/LocalElectronPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,22 @@ export default class LocalElectronPlugin extends PluginBase<LocalElectronPluginC
super(c);

this.getHooks = this.getHooks.bind(this);
this.startLogic = this.startLogic.bind(this);
}

init = (): void => {
if (this.enabled) {
this.checkPlatform(process.platform);
process.env.ELECTRON_OVERRIDE_DIST_PATH = this.config.electronPath;
}
};

Comment on lines +16 to +22
Copy link
Copy Markdown
Member

@BlackHole1 BlackHole1 Jun 28, 2023

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
Copy Markdown
Member Author

@reitowo reitowo Jun 28, 2023

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
Copy Markdown
Member

@BlackHole1 BlackHole1 Jun 28, 2023

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

get enabled(): boolean {
if (typeof this.config.enabled === 'undefined') {
return true;
}
return this.config.enabled;
}

async startLogic(): Promise<false> {
if (this.enabled) {
this.checkPlatform(process.platform);
process.env.ELECTRON_OVERRIDE_DIST_PATH = this.config.electronPath;
}
return false;
}

getHooks(): ForgeHookMap {
return {
packageAfterExtract: this.afterExtract,
Expand Down
22 changes: 8 additions & 14 deletions packages/plugin/local-electron/test/LocalElectronPlugin_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,30 +20,20 @@ describe('LocalElectronPlugin', () => {
it('should set ELECTRON_OVERRIDE_DIST_PATH when enabled', async () => {
expect(process.env.ELECTRON_OVERRIDE_DIST_PATH).to.equal(undefined);
const p = new LocalElectronPlugin({ electronPath: 'test/foo' });
await p.startLogic();
p.init();
expect(process.env.ELECTRON_OVERRIDE_DIST_PATH).to.equal('test/foo');
});

it('should not set ELECTRON_OVERRIDE_DIST_PATH when disabled', async () => {
expect(process.env.ELECTRON_OVERRIDE_DIST_PATH).to.equal(undefined);
const p = new LocalElectronPlugin({ enabled: false, electronPath: 'test/foo' });
await p.startLogic();
p.init();
expect(process.env.ELECTRON_OVERRIDE_DIST_PATH).to.equal(undefined);
});

it("should throw an error if platforms don't match", async () => {
const p = new LocalElectronPlugin({ electronPath: 'test/bar', electronPlatform: 'wut' });
await expect(p.startLogic()).to.eventually.be.rejectedWith(
`Can not use local Electron version, required platform "${process.platform}" but local platform is "wut"`
);
});

it('should always return false', async () => {
let p = new LocalElectronPlugin({ electronPath: 'test/bar' });
expect(await p.startLogic()).to.equal(false);

p = new LocalElectronPlugin({ enabled: false, electronPath: 'test/bar' });
expect(await p.startLogic()).to.equal(false);
expect(p.init).to.throw(`Can not use local Electron version, required platform "${process.platform}" but local platform is "wut"`);
});
});

Expand All @@ -53,7 +43,11 @@ describe('LocalElectronPlugin', () => {
beforeEach(() => {
p = new LocalElectronPlugin({ electronPath: 'test/foo' });
// eslint-disable-next-line @typescript-eslint/no-explicit-any
p.init('', {} as any);
p.init();
});

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

@BlackHole1 BlackHole1 Jun 28, 2023

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;
});

Copy link
Copy Markdown
Member Author

@reitowo reitowo Jun 28, 2023

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
Copy Markdown
Member

@BlackHole1 BlackHole1 Jun 28, 2023

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?

});

describe('with afterExtract hook', () => {
Expand Down