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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: better internal type safety for hooks #2995

Merged
merged 1 commit into from
Oct 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,5 @@ packages/api/cli/README.md
packages/**/tsconfig.tsbuildinfo
packages/**/.swc
.webpack
packages/api/core/test/fixture/app-with-scoped-name/out/make
packages/plugin/webpack/test/fixtures/apps/native-modules/package-lock.json
31 changes: 23 additions & 8 deletions packages/api/core/src/util/hook.ts
Original file line number Diff line number Diff line change
@@ -1,32 +1,47 @@
import { ResolvedForgeConfig } from '@electron-forge/shared-types';
import {
ForgeMutatingHookFn,
ForgeMutatingHookSignatures,
ForgeSimpleHookFn,
ForgeSimpleHookSignatures,
ResolvedForgeConfig,
} from '@electron-forge/shared-types';
import debug from 'debug';

const d = debug('electron-forge:hook');

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export const runHook = async (forgeConfig: ResolvedForgeConfig, hookName: string, ...hookArgs: any[]): Promise<void> => {
export const runHook = async <Hook extends keyof ForgeSimpleHookSignatures>(
forgeConfig: ResolvedForgeConfig,
hookName: Hook,
...hookArgs: ForgeSimpleHookSignatures[Hook]
): Promise<void> => {
const { hooks } = forgeConfig;
if (hooks) {
d(`hook triggered: ${hookName}`);
if (typeof hooks[hookName] === 'function') {
d('calling hook:', hookName, 'with args:', hookArgs);
await hooks[hookName](forgeConfig, ...hookArgs);
await (hooks[hookName] as ForgeSimpleHookFn<Hook>)(forgeConfig, ...hookArgs);
}
}
await forgeConfig.pluginInterface.triggerHook(hookName, hookArgs);
};

export async function runMutatingHook<T>(forgeConfig: ResolvedForgeConfig, hookName: string, item: T): Promise<T> {
export async function runMutatingHook<Hook extends keyof ForgeMutatingHookSignatures>(
forgeConfig: ResolvedForgeConfig,
hookName: Hook,
...item: ForgeMutatingHookSignatures[Hook]
): Promise<ForgeMutatingHookSignatures[Hook][0]> {
const { hooks } = forgeConfig;
if (hooks) {
d(`hook triggered: ${hookName}`);
if (typeof hooks[hookName] === 'function') {
d('calling mutating hook:', hookName, 'with item:', item);
const result = await hooks[hookName](forgeConfig, item);
d('calling mutating hook:', hookName, 'with item:', item[0]);
const hook = hooks[hookName] as ForgeMutatingHookFn<Hook>;
const result = await hook(forgeConfig, ...item);
if (typeof result !== 'undefined') {
item = result;
item[0] = result;
}
}
}
return forgeConfig.pluginInterface.triggerMutatingHook(hookName, item);
return forgeConfig.pluginInterface.triggerMutatingHook(hookName, item[0]);
}
32 changes: 22 additions & 10 deletions packages/api/core/src/util/plugin-interface.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
import { PluginBase } from '@electron-forge/plugin-base';
import { IForgePlugin, IForgePluginInterface, ResolvedForgeConfig, StartResult } from '@electron-forge/shared-types';
import {
ForgeMutatingHookFn,
ForgeMutatingHookSignatures,
ForgeSimpleHookFn,
ForgeSimpleHookSignatures,
IForgePlugin,
IForgePluginInterface,
ResolvedForgeConfig,
StartResult,
} from '@electron-forge/shared-types';
import debug from 'debug';

import { StartOptions } from '../api';
Expand Down Expand Up @@ -56,26 +65,29 @@ export default class PluginInterface implements IForgePluginInterface {
this.overrideStartLogic = this.overrideStartLogic.bind(this);
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
async triggerHook(hookName: string, hookArgs: any[]): Promise<void> {
async triggerHook<Hook extends keyof ForgeSimpleHookSignatures>(hookName: Hook, hookArgs: ForgeSimpleHookSignatures[Hook]): Promise<void> {
for (const plugin of this.plugins) {
if (typeof plugin.getHook === 'function') {
const hook = plugin.getHook(hookName);
if (typeof plugin.getHooks === 'function') {
const hook = plugin.getHooks()[hookName] as ForgeSimpleHookFn<Hook>;
if (hook) await hook(this.config, ...hookArgs);
}
}
}

async triggerMutatingHook<T>(hookName: string, item: T): Promise<T> {
async triggerMutatingHook<Hook extends keyof ForgeMutatingHookSignatures>(
hookName: Hook,
...item: ForgeMutatingHookSignatures[Hook]
): Promise<ForgeMutatingHookSignatures[Hook][0]> {
let result: ForgeMutatingHookSignatures[Hook][0] = item[0];
for (const plugin of this.plugins) {
if (typeof plugin.getHook === 'function') {
const hook = plugin.getHook(hookName);
if (typeof plugin.getHooks === 'function') {
const hook = plugin.getHooks()[hookName] as ForgeMutatingHookFn<Hook>;
if (hook) {
item = await hook(this.config, item);
result = (await hook(this.config, ...item)) || result;
}
}
}
return item;
return result;
}

async overrideStartLogic(opts: StartOptions): Promise<StartResult> {
Expand Down
30 changes: 22 additions & 8 deletions packages/api/core/test/fast/hook_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,33 +14,47 @@ const fakeConfig = {
describe('hooks', () => {
describe('runHook', () => {
it('should not error when running non existent hooks', async () => {
await runHook({ ...fakeConfig }, 'magic');
await runHook({ ...fakeConfig }, 'preMake');
});

it('should not error when running a hook that is not a function', async () => {
await runHook({ hooks: { myHook: 'abc' as unknown as ForgeHookFn }, ...fakeConfig }, 'abc');
await runHook({ hooks: { preMake: 'abc' as unknown as ForgeHookFn<'preMake'> }, ...fakeConfig }, 'preMake');
});

it('should run the hook if it is provided as a function', async () => {
const myStub = stub();
myStub.returns(Promise.resolve());
await runHook({ hooks: { myHook: myStub }, ...fakeConfig }, 'myHook');
await runHook({ hooks: { preMake: myStub }, ...fakeConfig }, 'preMake');
expect(myStub.callCount).to.equal(1);
});
});

describe('runMutatingHook', () => {
it('should return the input when running non existent hooks', async () => {
expect(await runMutatingHook({ ...fakeConfig }, 'magic', 'input')).to.equal('input');
const info = {
foo: 'bar',
};
expect(await runMutatingHook({ ...fakeConfig }, 'readPackageJson', info)).to.equal(info);
});

it('should return the mutated input when returned from a hook', async () => {
fakeConfig.pluginInterface.triggerMutatingHook = stub().returnsArg(1);
const myStub = stub();
myStub.returns(Promise.resolve('magneto'));
const output = await runMutatingHook({ hooks: { myHook: myStub }, ...fakeConfig }, 'myHook', 'input');
expect(output).to.equal('magneto');
expect((fakeConfig.pluginInterface.triggerMutatingHook as SinonStub).firstCall.args[1]).to.equal('magneto');
myStub.returns(
Promise.resolve({
mutated: 'foo',
})
);
const info = {
foo: 'bar',
};
const output = await runMutatingHook({ hooks: { readPackageJson: myStub }, ...fakeConfig }, 'readPackageJson', info);
expect(output).to.deep.equal({
mutated: 'foo',
});
expect((fakeConfig.pluginInterface.triggerMutatingHook as SinonStub).firstCall.args[1]).to.deep.equal({
mutated: 'foo',
});
});
});
});
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
import { PluginBase } from '@electron-forge/plugin-base';
import { ForgeHookFn, ResolvedForgeConfig } from '@electron-forge/shared-types';
import { ForgeHookFn, ForgeHookMap } from '@electron-forge/shared-types';

import { AutoUnpackNativesConfig } from './Config';

export default class AutoUnpackNativesPlugin extends PluginBase<AutoUnpackNativesConfig> {
name = 'auto-unpack-natives';

getHook(hookName: string): ForgeHookFn | null {
if (hookName === 'resolveForgeConfig') {
return this.resolveForgeConfig;
}
return null;
getHooks(): ForgeHookMap {
return {
resolveForgeConfig: this.resolveForgeConfig,
};
}

resolveForgeConfig = async (forgeConfig: ResolvedForgeConfig): Promise<ResolvedForgeConfig> => {
resolveForgeConfig: ForgeHookFn<'resolveForgeConfig'> = async (forgeConfig) => {
if (!forgeConfig.packagerConfig) {
forgeConfig.packagerConfig = {};
}
Expand Down
13 changes: 9 additions & 4 deletions packages/plugin/base/src/Plugin.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ElectronProcess, ForgeHookFn, IForgePlugin, ResolvedForgeConfig, StartOptions } from '@electron-forge/shared-types';
import { ElectronProcess, ForgeHookMap, IForgePlugin, ResolvedForgeConfig, StartOptions } from '@electron-forge/shared-types';

export { StartOptions };

Expand All @@ -7,6 +7,8 @@ export default abstract class Plugin<C> implements IForgePlugin {

/** @internal */
__isElectronForgePlugin!: true;
/** @internal */
_resolvedHooks: ForgeHookMap = {};

constructor(public config: C) {
Object.defineProperty(this, '__isElectronForgePlugin', {
Expand All @@ -17,11 +19,14 @@ export default abstract class Plugin<C> implements IForgePlugin {
}

init(_dir: string, _config: ResolvedForgeConfig): void {
// By default, do nothing. This can be overridden.
// This logic ensures that we only call getHooks once regardless of how many
// times we trip hook logic in the PluginInterface.
this._resolvedHooks = this.getHooks();
this.getHooks = () => this._resolvedHooks;
}

getHook(_hookName: string): ForgeHookFn | null {
return null;
getHooks(): ForgeHookMap {
return {};
}

async startLogic(_startOpts: StartOptions): Promise<ElectronProcess | string | string[] | false> {
Expand Down
16 changes: 8 additions & 8 deletions packages/plugin/compile/src/CompilePlugin.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as path from 'path';

import { PluginBase, StartOptions } from '@electron-forge/plugin-base';
import { ForgeHookFn } from '@electron-forge/shared-types';
import { ForgeHookMap, ResolvedForgeConfig } from '@electron-forge/shared-types';

import { CompilePluginConfig } from './Config';
import { createCompileHook } from './lib/compile-hook';
Expand All @@ -15,19 +15,19 @@ export default class CompileElectronPlugin extends PluginBase<CompilePluginConfi
super(c);

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

init(dir: string): void {
init(dir: string, config: ResolvedForgeConfig): void {
super.init(dir, config);
this.dir = dir;
}

getHook(hookName: string): ForgeHookFn | null {
if (hookName === 'packageAfterCopy') {
return createCompileHook(this.dir);
}
return null;
getHooks(): ForgeHookMap {
return {
packageAfterCopy: createCompileHook(this.dir),
};
}

async startLogic(_opts: StartOptions): Promise<string[]> {
Expand Down
6 changes: 3 additions & 3 deletions packages/plugin/compile/src/lib/compile-hook.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import path from 'path';

import { asyncOra } from '@electron-forge/async-ora';
import { ResolvedForgeConfig } from '@electron-forge/shared-types';
import { ForgeHookFn } from '@electron-forge/shared-types';
import fs from 'fs-extra';

export const createCompileHook =
(originalDir: string) =>
async (_config: ResolvedForgeConfig, buildPath: string): Promise<void> => {
(originalDir: string): ForgeHookFn<'packageAfterCopy'> =>
async (_config, buildPath): Promise<void> => {
await asyncOra('Compiling Application', async () => {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const compileCLI = require(path.resolve(originalDir, 'node_modules/electron-compile/lib/cli.js'));
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
import runElectronegativity from '@doyensec/electronegativity';
import { PluginBase } from '@electron-forge/plugin-base';
import { ForgeHookFn, ResolvedForgeConfig } from '@electron-forge/shared-types';

// To be more precise, postPackage options we care about.
type PostPackageOptions = {
outputPaths: string[];
};
import { ForgeHookFn, ForgeHookMap } from '@electron-forge/shared-types';

export type Confidence = 'certain' | 'firm' | 'tentative';
export type CustomCheck = 'dangerousfunctionsjscheck' | 'remotemodulejscheck';
Expand Down Expand Up @@ -60,14 +55,13 @@ export type ElectronegativityConfig = {
export default class ElectronegativityPlugin extends PluginBase<ElectronegativityConfig> {
name = 'electronegativity';

getHook(hookName: string): ForgeHookFn | null {
if (hookName === 'postPackage') {
return this.postPackage;
}
return null;
getHooks(): ForgeHookMap {
return {
postPackage: this.postPackage,
};
}

postPackage = async (_forgeConfig: ResolvedForgeConfig, options: PostPackageOptions): Promise<void> => {
postPackage: ForgeHookFn<'postPackage'> = async (_forgeConfig, options): Promise<void> => {
await runElectronegativity(
{
...this.config,
Expand Down
15 changes: 7 additions & 8 deletions packages/plugin/local-electron/src/LocalElectronPlugin.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { PluginBase } from '@electron-forge/plugin-base';
import { ForgeHookFn, ResolvedForgeConfig } from '@electron-forge/shared-types';
import { ForgeHookFn, ForgeHookMap } from '@electron-forge/shared-types';
import fs from 'fs-extra';

import { LocalElectronPluginConfig } from './Config';
Expand All @@ -10,7 +10,7 @@ export default class LocalElectronPlugin extends PluginBase<LocalElectronPluginC
constructor(c: LocalElectronPluginConfig) {
super(c);

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

Expand All @@ -29,11 +29,10 @@ export default class LocalElectronPlugin extends PluginBase<LocalElectronPluginC
return false;
}

getHook(hookName: string): ForgeHookFn | null {
if (hookName === 'packageAfterExtract') {
return this.afterExtract;
}
return null;
getHooks(): ForgeHookMap {
return {
packageAfterExtract: this.afterExtract,
};
}

private checkPlatform = (platform: string) => {
Expand All @@ -50,7 +49,7 @@ export default class LocalElectronPlugin extends PluginBase<LocalElectronPluginC
}
};

private afterExtract = async (_config: ResolvedForgeConfig, buildPath: string, _electronVersion: string, platform: string, arch: string) => {
private afterExtract: ForgeHookFn<'packageAfterExtract'> = async (_config, buildPath, _electronVersion, platform, arch) => {
if (!this.enabled) return;

this.checkPlatform(platform);
Expand Down