feat: [#1502] Allow fetch to be intercepted and modified #1662

OlaviSau · 2025-01-03T11:17:10Z

A proposal for implementation.
The intercept option is structured a bit deep, but it is for a reason. Since there exists a SyncFetch as well I could not come up with a good name that would clearly imply that the interceptor only applies for async requests and not for sync requests.
I considered the following options:
{ fetch: { beforeSend: ... } }
This is confusing as there is SyncFetch and Fetch. The types wouldn't match either for the response object. In addition if the options were to be a class, the method would be lost during setup in quite a confusing way.
{ fetch: { beforeAsync: ... } }
This doesn't clarify before what and has the issue of losing the methods.
{ fetch: { asyncInterceptor: { beforeSend: ... } } }
The use might assume that the interceptor is allowed to be async and applies both to SyncFetch and Fetch. Even if we change it to asyncFetchInterceptor it retains that problem.

Methods can be lost during spreading in BrowserSettingsFactory.createSettings(options?.settings) as methods from classes are not enumerable by default.

I also considered whether we could use the interceptor for setting the options instead of using the declarative way - it would allow for dynamically setting some options on fetch like disableCache, redirectCount or disableSameOriginPolicy. The downside is that it would require exposing the Fetch type to some extent. I'll leave that decision to @capricorn86.
The window is exposed due to the need for window for creating the response object as the response object cannot be created independently of the window. That would cause problems when creating the interceptor before the Window, which should be the default use case. It might also be useful if the interceptor is used for multiple windows.

If this approach is suitable - I can add a hooks for afterResponse, afterError and other cases that the users would like.

capricorn86 · 2025-01-03T14:09:56Z

I think your approach is good @OlaviSau 🙂

I agree that there are many approaches with their pros and cons and it is not easy to say which one is the best.

After thinking about it, my suggestion is:

IFetchInterceptor.ts

export default interface IFetchInterceptor {
   beforeRequest?: (window: BrowserWindow, request: Request) => Promise<Response | void>;
   beforeSyncRequest?: (window: BrowserWindow, request: Request) => ISyncResponse | void;
   afterResponse? (window: BrowserWindow, request: Request, response: Response) => Promise<Response | void>;
   afterSyncResponse? (window: BrowserWindow, request: Request, response: ISyncResponse) => Promise<ISyncResponse | void>;
}

IBrowserSettings.ts

export default interface IBrowserSettings {
   fetch: {
      interceptor?: IFetchInterceptor
   };
}

I'm not sure "beforeRequest" is better than "beforeSend", but looking at some other libs it seems like it is common to use "beforeRequest" and "afterResponse".

I think it will make the code easier to understand and maintain if all interceptor logic is within the IFetchInterceptor interface (both for async and sync). I'm thinking that async is the default fetch behavior, so it might not need to be prefixed with "Async".

OlaviSau · 2025-01-04T00:11:29Z

Thanks for the response ❤️
I think beforeRequest might be better if it's more common, now that I think about it - beforeRequest is probably more intuitive than beforeSend thinking from the outside context rather than inside the fetch class.

In terms of remove the the asyncFetch - I think that also makes sense.
There are 2 other changes you're suggesting though that I would like to get your reasoning behind window before the request. I was thinking that the window will primarily be useful when the user wants to return a response, but since that's not a guaranteed case - the request would be first.
The cases I am thinking about.

The user wants to do logging. Potentially require any number of parameters with window / request / response being unknown.
The user wants to change the request. Requires request.
The user wants to provide a response before the request is made. Requires request and window.
The user wants to change the response. Requires response and most likely the request, but not always (header addition).
The user wants to replace the response based on the response. Requires response, request and window.

It seems that it varies quite a bit what the user needs, perhaps I should use a context object instead?

I thought a bit about the extra layer and now I actually think it's fine that they are all in the same interceptor as the user can use composition on their class if they want to split functionality / handle data persistence in a shared manner. That allows me to remove one layer from the settings so I am happy about that :)

OlaviSau · 2025-01-04T00:52:21Z

I added sync request for now as well. Will add response hooks soon.

capricorn86 · 2025-01-04T00:54:44Z

Thanks for the response ❤️ I think beforeRequest might be better if it's more common, now that I think about it - beforeRequest is probably more intuitive than beforeSend thinking from the outside context rather than inside the fetch class.

In terms of remove the the asyncFetch - I think that also makes sense. There are 2 other changes you're suggesting though that I would like to get your reasoning behind window before the request. I was thinking that the window will primarily be useful when the user wants to return a response, but since that's not a guaranteed case - the request would be first. The cases I am thinking about.

The user wants to do logging. Potentially require any number of parameters with window / request / response being unknown.

The user wants to change the request. Requires request.

The user wants to provide a response before the request is made. Requires request and window.

The user wants to change the response. Requires response and most likely the request, but not always (header addition).

The user wants to replace the response based on the response. Requires response, request and window.

It seems that it varies quite a bit what the user needs, perhaps I should use a context object instead?

I thought a bit about the extra layer and now I actually think it's fine that they are all in the same interceptor as the user can use composition on their class if they want to split functionality / handle data persistence in a shared manner. That allows me to remove one layer from the settings so I am happy about that :)

I'm glad to hear that you agree for the most part with my suggestion 😄

I get your point. I know that many libraries would send the parameters in that order.

I see it as the injection pattern where the context is the most important parameter that always has to be sent no matter which function, and request is the second to know which request it is about. This way we know that the first parameter will always be the same, instead of not knowing where the window parameter is.

Example:
beforeRequest?: (request: Request, window: BrowserWindow, options?: Object) => Promise<Response | void>;
afterResponse?: (request: Request, response: Response, window: BrowserWindow, options?: Object => Promise<Response | void>;
beforeError?: (error: Error, request: Request, response: Response | null, window: BrowserWindow, options?: Object => Promise<Response | void>;

This is how most logic works for other Happy DOM functionality as much logic needs to know which Window context it is part of, specially when dealing with multiple Window instances running at the same time. The consumer will most likely need to know which Window at some point. It could for example be for checking the origin of the request as it can potentially be an iframe or another page.

Another way as I believe you suggested is to send in some kind of object containing the properties. Objects are nice for the consumer, except if they require custom typings, but perhaps it isn't that big of a problem.

I hope I explained my thinking a little bit 😅

OlaviSau · 2025-01-04T01:26:59Z

I see, yes, that's a good point, then the window would be at a predictable location. I think in that case in order to have consistency, but also contend with only needing certain parameters at different times the object seems to be a good solution. One note - I exposed ISyncResponse so that the user would be able to type their responses correctly.

OlaviSau · 2025-01-06T12:48:04Z

packages/happy-dom/src/fetch/Fetch.ts

@@ -39,7 +40,7 @@ const LAST_CHUNK = Buffer.from('0\r\n\r\n');
 */
 export default class Fetch {
 	private reject: (reason: Error) => void | null = null;
-	private resolve: (value: Response | Promise<Response>) => void | null = null;
+	private resolve: (value: Response | Promise<Response>) => Promise<void> = null;


Could this cause any issues? @capricorn86

It is only used internally, so it should be fine

OlaviSau · 2025-01-06T12:54:16Z

@capricorn86 I have added the response methods as well. I am noticing that there is quite a bit of duplication in the tests for the Fetch / SyncFetch now. Should I try to eliminate some of it or is it okay?

OlaviSau · 2025-01-06T13:25:14Z

One other thing I was thinking about was perhaps splitting the afterResponse into two - beforeCache and afterCache. The naming would be tricky though so I just decided to leave it with the current plan.

capricorn86

Looks good! 🙂 Just a small finding

capricorn86 · 2025-01-07T14:31:25Z

packages/happy-dom/src/fetch/Fetch.ts

@@ -383,7 +402,14 @@ export default class Fetch {
 						});
 				}
 				this.#browserFrame[PropertySymbol.asyncTaskManager].endTask(taskID);


We need to end the task for the "asyncTaskManager" after the intercepted response has been handled

Fixed it :)

capricorn86 · 2025-01-07T14:35:24Z

packages/happy-dom/src/fetch/Fetch.ts

@@ -39,7 +40,7 @@ const LAST_CHUNK = Buffer.from('0\r\n\r\n');
 */
 export default class Fetch {
 	private reject: (reason: Error) => void | null = null;
-	private resolve: (value: Response | Promise<Response>) => void | null = null;
+	private resolve: (value: Response | Promise<Response>) => Promise<void> = null;


It is only used internally, so it should be fine

capricorn86 · 2025-01-07T14:40:40Z

@capricorn86 I have added the response methods as well. I am noticing that there is quite a bit of duplication in the tests for the Fetch / SyncFetch now. Should I try to eliminate some of it or is it okay?

As long as the code is still easy to read and understand after removing any duplication you can try to eliminate some of it. To me it is better with some duplication and more code if it makes it easier to understand it.

OlaviSau · 2025-01-07T16:52:02Z

@capricorn86 I have added the response methods as well. I am noticing that there is quite a bit of duplication in the tests for the Fetch / SyncFetch now. Should I try to eliminate some of it or is it okay?

As long as the code is still easy to read and understand after removing any duplication you can try to eliminate some of it. To me it is better with some duplication and more code if it makes it easier to understand it.

I thought about it a bit and I could replace the mockModule part with a function that would return a promise that then returns the requestArgs. If possible I would like to do this in a secondary PR for easier review.

This is what I had in mind.

	async function mockNetwork(
		schema: 'http' | 'https',
		responseText: string,
		extra: Record<string, unknown>
	): Promise<{ url: string; options: HTTP.RequestOptions }> {
		return new Promise((resolve) => {
			mockModule(schema, {
				request: (url: string, options: HTTP.RequestOptions) => {
					return {
						end: () => {},
						on: (event: string, callback: (response: HTTP.IncomingMessage) => void) => {
							async function* generate(): AsyncGenerator<string> {
								yield responseText;
							}

							const response = <HTTP.IncomingMessage>Stream.Readable.from(generate());
							Object.assign(response, extra);
							if (event === 'response') {
								callback(response);
							}
							resolve({ url, options });
						},
						setTimeout: () => {}
					};
				}
			});
		});
	}

The consumption would look something like

const requestArgsPromise = mockNetwork(
	'http',
	'some text',
	{
		statusCode: 200,
		statusMessage: 'OK',
		headers: {},
		rawHeaders: [
			'content-type',
			'text/html',
			'content-length',
			String(responseText.length)
		]
	}
);
// fetch
const requestArgs = await requestArgsPromise;

I tested it with some tests, seems that the GH workflow doesn't always run all tests, so I caught a bug I added at some point by using the interceptor without optional access.

…om:OlaviSau/happy-dom into feature-1502-allow-fetch-to-be-intercepted

capricorn86 · 2025-01-07T21:57:54Z

@capricorn86 I have added the response methods as well. I am noticing that there is quite a bit of duplication in the tests for the Fetch / SyncFetch now. Should I try to eliminate some of it or is it okay?

As long as the code is still easy to read and understand after removing any duplication you can try to eliminate some of it. To me it is better with some duplication and more code if it makes it easier to understand it.

I thought about it a bit and I could replace the mockModule part with a function that would return a promise that then returns the requestArgs. If possible I would like to do this in a secondary PR for easier review.

This is what I had in mind.
	async function mockNetwork(
		schema: 'http' | 'https',
		responseText: string,
		extra: Record<string, unknown>
	): Promise<{ url: string; options: HTTP.RequestOptions }> {
		return new Promise((resolve) => {
			mockModule(schema, {
				request: (url: string, options: HTTP.RequestOptions) => {
					return {
						end: () => {},
						on: (event: string, callback: (response: HTTP.IncomingMessage) => void) => {
							async function* generate(): AsyncGenerator<string> {
								yield responseText;
							}

							const response = <HTTP.IncomingMessage>Stream.Readable.from(generate());
							Object.assign(response, extra);
							if (event === 'response') {
								callback(response);
							}
							resolve({ url, options });
						},
						setTimeout: () => {}
					};
				}
			});
		});
	}
The consumption would look something like
const requestArgsPromise = mockNetwork(
	'http',
	'some text',
	{
		statusCode: 200,
		statusMessage: 'OK',
		headers: {},
		rawHeaders: [
			'content-type',
			'text/html',
			'content-length',
			String(responseText.length)
		]
	}
);
// fetch
const requestArgs = await requestArgsPromise;
I tested it with some tests, seems that the GH workflow doesn't always run all tests, so I caught a bug I added at some point by using the interceptor without optional access.

Sounds good! It could probably clean it up a bit for a majority of the tests 🙂 It might not be possible to use it for some of the tests (like tests for post or chunks).

OlaviSau requested a review from capricorn86 as a code owner January 3, 2025 11:17

OlaviSau mentioned this pull request Jan 3, 2025

Intercept network resource requests / defining resource loader #1502

Closed

feat: [1502] Allow fetch to be intercepted and modified

a9ec9d8

OlaviSau force-pushed the feature-1502-allow-fetch-to-be-intercepted branch from e95910f to a9ec9d8 Compare January 3, 2025 11:22

OlaviSau force-pushed the feature-1502-allow-fetch-to-be-intercepted branch from 0184a4f to ea4f90f Compare January 4, 2025 00:50

feat: [1502] Add the possibility to intercept sync requests

050bab7

OlaviSau force-pushed the feature-1502-allow-fetch-to-be-intercepted branch from ea4f90f to 050bab7 Compare January 4, 2025 00:51

fix: [1502] Correct the types for sync response hooks

fdda4ed

OlaviSau commented Jan 6, 2025

View reviewed changes

feat: [1502] Allow fetch response to be intercepted

c5f8b26

OlaviSau force-pushed the feature-1502-allow-fetch-to-be-intercepted branch from b606383 to c5f8b26 Compare January 6, 2025 12:52

capricorn86 requested changes Jan 7, 2025

View reviewed changes

OlaviSau added 2 commits January 7, 2025 18:21

fix: [1502] Move async task manager to be ended after the intercept

3717627

Merge branch 'master' into feature-1502-allow-fetch-to-be-intercepted

700ff3e

OlaviSau added 2 commits January 7, 2025 19:03

fix: [1502] The interceptor not always being there

56c8c97

Merge branch 'feature-1502-allow-fetch-to-be-intercepted' of github.c…

6e4b68d

…om:OlaviSau/happy-dom into feature-1502-allow-fetch-to-be-intercepted

capricorn86 approved these changes Jan 7, 2025

View reviewed changes

capricorn86 changed the title ~~feat: [1502] Allow fetch to be intercepted and modified~~ feat: [#1502] Allow fetch to be intercepted and modified Jan 7, 2025

capricorn86 merged commit 4975dc5 into capricorn86:master Jan 7, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [#1502] Allow fetch to be intercepted and modified #1662

feat: [#1502] Allow fetch to be intercepted and modified #1662

OlaviSau commented Jan 3, 2025 •

edited

Loading

capricorn86 commented Jan 3, 2025 •

edited

Loading

OlaviSau commented Jan 4, 2025

OlaviSau commented Jan 4, 2025

capricorn86 commented Jan 4, 2025 •

edited

Loading

OlaviSau commented Jan 4, 2025

OlaviSau Jan 6, 2025 •

edited

Loading

capricorn86 Jan 7, 2025

OlaviSau commented Jan 6, 2025

OlaviSau commented Jan 6, 2025

capricorn86 left a comment

capricorn86 Jan 7, 2025

OlaviSau Jan 7, 2025

capricorn86 Jan 7, 2025

capricorn86 commented Jan 7, 2025

OlaviSau commented Jan 7, 2025 •

edited

Loading

capricorn86 commented Jan 7, 2025 •

edited

Loading

feat: [#1502] Allow fetch to be intercepted and modified #1662

feat: [#1502] Allow fetch to be intercepted and modified #1662

Conversation

OlaviSau commented Jan 3, 2025 • edited Loading

capricorn86 commented Jan 3, 2025 • edited Loading

OlaviSau commented Jan 4, 2025

OlaviSau commented Jan 4, 2025

capricorn86 commented Jan 4, 2025 • edited Loading

OlaviSau commented Jan 4, 2025

OlaviSau Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

capricorn86 Jan 7, 2025

Choose a reason for hiding this comment

OlaviSau commented Jan 6, 2025

OlaviSau commented Jan 6, 2025

capricorn86 left a comment

Choose a reason for hiding this comment

capricorn86 Jan 7, 2025

Choose a reason for hiding this comment

OlaviSau Jan 7, 2025

Choose a reason for hiding this comment

capricorn86 Jan 7, 2025

Choose a reason for hiding this comment

capricorn86 commented Jan 7, 2025

OlaviSau commented Jan 7, 2025 • edited Loading

capricorn86 commented Jan 7, 2025 • edited Loading

OlaviSau commented Jan 3, 2025 •

edited

Loading

capricorn86 commented Jan 3, 2025 •

edited

Loading

capricorn86 commented Jan 4, 2025 •

edited

Loading

OlaviSau Jan 6, 2025 •

edited

Loading

OlaviSau commented Jan 7, 2025 •

edited

Loading

capricorn86 commented Jan 7, 2025 •

edited

Loading