Repository: swup/swup
Pull request #849 (2023-12-10 10:40:12)

Ignore errors in user hooks
**Description**

- Attempt to tackle #848 
- Make swup more forgiving when it comes to errors in user code
- Catch and log exceptions in user hooks to keep visits running
- Only throw and force-reload for exceptions in internal default handler

**Scenario**

- Mistyped a variable name in a user hook → swup now always reloads, making this hard to figure out in the console

**Checks**

- [x] The PR is submitted to the `master` branch
- [x] The code was linted before pushing (`npm run lint`)
- [x] All tests are passing (`npm run test`)
- [x] New or updated tests are included
- [ ] ~~The documentation was updated as required~~



Patch:
diff --git a/src/modules/Hooks.ts b/src/modules/Hooks.ts
index f716592..36af2bf 100644
--- a/src/modules/Hooks.ts
+++ b/src/modules/Hooks.ts
@@ -354,7 +354,7 @@ export class Hooks {
 
 		const { before, handler, after } = this.getHandlers(hook, defaultHandler);
 		await this.run(before, visit, args);
-		const [result] = await this.run(handler, visit, args);
+		const [result] = await this.run(handler, visit, args, true);
 		await this.run(after, visit, args);
 		this.dispatchDomEvent(hook, visit, args);
 		return result;
@@ -383,7 +383,7 @@ export class Hooks {
 		const [visit, args, defaultHandler] = this.parseCallArgs(hook, arg1, arg2, arg3);
 		const { before, handler, after } = this.getHandlers(hook, defaultHandler);
 		this.runSync(before, visit, args);
-		const [result] = this.runSync(handler, visit, args);
+		const [result] = this.runSync(handler, visit, args, true);
 		this.runSync(after, visit, args);
 		this.dispatchDomEvent(hook, visit, args);
 		return result;
@@ -416,25 +416,30 @@ export class Hooks {
 	 */
 
 	// Overload: running HookDefaultHandler: expect HookDefaultHandler return type
-	protected async run<T extends HookName>(registrations: HookRegistration<T, HookDefaultHandler<T>>[], visit: Visit | undefined, args: HookArguments<T>): Promise<Awaited<ReturnType<HookDefaultHandler<T>>>[]>; // prettier-ignore
+	protected async run<T extends HookName>(registrations: HookRegistration<T, HookDefaultHandler<T>>[], visit: Visit | undefined, args: HookArguments<T>, rethrow: true): Promise<Awaited<ReturnType<HookDefaultHandler<T>>>[]>; // prettier-ignore
 	// Overload:  running user handler: expect no specific type
 	protected async run<T extends HookName>(registrations: HookRegistration<T>[], visit: Visit | undefined, args: HookArguments<T>): Promise<unknown[]>; // prettier-ignore
 	// Implementation
 	protected async run<T extends HookName, R extends HookRegistration<T>[]>(
 		registrations: R,
-		visit: Visit | undefined,
-		args: HookArguments<T>
+		visit: Visit | undefined = this.swup.visit,
+		args: HookArguments<T>,
+		rethrow: boolean = false
 	): Promise<Awaited<ReturnType<HookDefaultHandler<T>>> | unknown[]> {
 		const results = [];
 		for (const { hook, handler, defaultHandler, once } of registrations) {
 			if (visit?.done) continue;
 			if (once) this.off(hook, handler);
-			const result = await runAsPromise(handler, [
-				visit || this.swup.visit,
-				args,
-				defaultHandler
-			]);
-			results.push(result);
+			try {
+				const result = await runAsPromise(handler, [visit, args, defaultHandler]);
+				results.push(result);
+			} catch (error) {
+				if (rethrow) {
+					throw error;
+				} else {
+					console.error(`Error in hook '${hook}':`, error);
+				}
+			}
 		}
 		return results;
 	}
@@ -446,26 +451,34 @@ export class Hooks {
 	 */
 
 	// Overload: running HookDefaultHandler: expect HookDefaultHandler return type
-	protected runSync<T extends HookName>(registrations: HookRegistration<T, HookDefaultHandler<T>>[], visit: Visit | undefined, args: HookArguments<T> ): ReturnType<HookDefaultHandler<T>>[]; // prettier-ignore
+	protected runSync<T extends HookName>(registrations: HookRegistration<T, HookDefaultHandler<T>>[], visit: Visit | undefined, args: HookArguments<T>, rethrow: true): ReturnType<HookDefaultHandler<T>>[]; // prettier-ignore
 	// Overload: running user handler: expect no specific type
 	protected runSync<T extends HookName>(registrations: HookRegistration<T>[], visit: Visit | undefined, args: HookArguments<T>): unknown[]; // prettier-ignore
 	// Implementation
 	protected runSync<T extends HookName, R extends HookRegistration<T>[]>(
 		registrations: R,
-		visit: Visit | undefined,
-		args: HookArguments<T>
+		visit: Visit | undefined = this.swup.visit,
+		args: HookArguments<T>,
+		rethrow: boolean = false
 	): (ReturnType<HookDefaultHandler<T>> | unknown)[] {
 		const results = [];
 		for (const { hook, handler, defaultHandler, once } of registrations) {
 			if (visit?.done) continue;
 			if (once) this.off(hook, handler);
-			const result = (handler as HookDefaultHandler<T>)(visit || this.swup.visit, args, defaultHandler); // prettier-ignore
-			results.push(result);
-			if (isPromise(result)) {
-				console.warn(
-					`Promise returned from handler for synchronous hook '${hook}'.` +
-						`Swup will not wait for it to resolve.`
-				);
+			try {
+				const result = (handler as HookDefaultHandler<T>)(visit, args, defaultHandler);
+				results.push(result);
+				if (isPromise(result)) {
+					console.warn(
+						`Swup will not await Promises in handler for synchronous hook '${hook}'.`
+					);
+				}
+			} catch (error) {
+				if (rethrow) {
+					throw error;
+				} else {
+					console.error(`Error in hook '${hook}':`, error);
+				}
 			}
 		}
 		return results;
diff --git a/src/modules/navigate.ts b/src/modules/navigate.ts
index 7e513d0..96ee57c 100755
--- a/src/modules/navigate.ts
+++ b/src/modules/navigate.ts
@@ -218,11 +218,10 @@ export async function performNavigation(
 
 		visit.state = VisitState.FAILED;
 
-		// Log to console as we swallow almost all hook errors
+		// Log to console
 		console.error(error);
 
 		// Remove current history entry, then load requested url in browser
-
 		this.options.skipPopStateHandling = () => {
 			window.location.assign(visit.to.url + visit.to.hash);
 			return true;


Test patch:
diff --git a/tests/unit/hooks.test.ts b/tests/unit/hooks.test.ts
index 1023468..3da6a0d 100644
--- a/tests/unit/hooks.test.ts
+++ b/tests/unit/hooks.test.ts
@@ -335,6 +335,28 @@ describe('Hook registry', () => {
 		expect(defaultHandler).toBeCalledTimes(1);
 		expect(defaultHandler).toHaveBeenNthCalledWith(1, visit, args, undefined);
 	});
+
+	it('should swallow exceptions in user handlers', async () => {
+		const swup = new Swup();
+		const handlerWithError = vi.fn(() => {
+			throw new Error('UserError');
+		});
+
+		swup.hooks.on('enable', handlerWithError);
+		await expect(swup.hooks.call('enable', undefined, undefined)).resolves.toBeUndefined();
+		expect(handlerWithError).toBeCalledTimes(1);
+	});
+
+	it('should re-throw exceptions in default handlers', async () => {
+		const swup = new Swup();
+		const handlerWithError = vi.fn(() => {
+			throw new Error('UserError');
+		});
+
+		swup.hooks.before('enable', handlerWithError); // run before default handler
+		await expect(() => swup.hooks.call('enable', undefined, undefined, handlerWithError)).rejects.toThrow(/^UserError$/);
+		expect(handlerWithError).toBeCalledTimes(2);
+	});
 });
 
 describe('Types', () => {
@@ -356,9 +378,6 @@ describe('Types', () => {
 		// @ts-expect-error: event arg must be PopStateEvent
 		await swup.hooks.call('history:popstate', undefined, { event: new MouseEvent('') });
 		// @ts-expect-error: handler arg must be optional: handler?
-		swup.hooks.replace(
-			'enable',
-			(visit: Visit, args: undefined, handler: HookDefaultHandler<'enable'>) => {}
-		);
+		swup.hooks.replace('enable', (visit: Visit, args: undefined, handler: HookDefaultHandler<'enable'>) => {});
 	});
 });