Refactor the "handleRequest" pipeline

[kettanaito]

Motivation

There are a few pain points when working with the handleRequest/getResponse functions internally:

1. The call signature of handleRequest is a mess:

msw/src/core/utils/handleRequest.ts

Lines 45 to 52 in b440f12

	export async function handleRequest(
	request: Request,
	requestId: string,
	handlers: Array<RequestHandler>,
	options: RequiredDeep<SharedOptions>,
	emitter: Emitter<LifeCycleEventsMap>,
	handleRequestOptions?: HandleRequestOptions,
	): Promise<Response | undefined> {

It accepts way too much (requestId needed just for emitter, the emitter itself, which is a huge side-effect that request resolution doesn't need at all) and it's generally poorly structured (does too much).

2. There isn't much benefit of keeping getResponse as a standalone function.

msw/src/core/utils/getResponse.ts

Lines 19 to 23 in b440f12

	export const getResponse = async <Handler extends Array<RequestHandler>>(
	request: Request,
	handlers: Handler,
	resolutionContext?: ResponseResolutionContext,
	): Promise<ResponseLookupResult | null> => {

It makes the request resolution logic feel more complex than it actually is. Granted, it used to be much larger, and after the V2 refactoring I should've just moved it into handleRequest.ts.

Proposed changes

Replace handleRequest with a new Network API

// SetupApi.ts
this.network = new Network({
  handlers: this.currentHandlers
})

// SetupServerApi.ts
interceptor.on('request', async ({ request }) => {
  const response = await this.network.resolve(request)
})

The names are due for discussion.

We would still have to work around propagating requestId because it's a serializable request identifier (unlike the Request instance).

Introduce cache

• Related to chore: cache parsed requests by request #1900

class Network {
  private cache: WeakMap<Request, T>
}

Add a cache property on the Network instance that would do the following:

• Cache the request parsing result to prevent parsing the same request N amount of times, where N is the number of all request handlers you have;
• Provide a generic interface to insert arbitrary cache (e.g. cookie parsing cache of the HttpHandler).

Refactor events (emitter)

Either make the Network emit its own events and pipe them to the setup API-based emitter, or make the network accept the said emitter directly.

[mattcosta7]

Overall this seems like a good direction!

Re cache - looking into this more, i'm not sure if we need to implement a generic one yet, or if we can implement something more 'as necessary' and make it more generic later if we find it valuable.

https://stackblitz.com/edit/vitest-dev-vitest-utw4vd?file=test%2Fbasic.test.ts

I put together a very very very naiive sample of the performance for a large set of handlers.

For HttpHandlers the different between the first and last handler in a set of 1000 is negligible (10ms for 1 vs 60ms for 1000). (Parsing cookies and path matching is fast!)

For GraphQLHandlers the first handler cost is about the same as 1000 HttpHandlers. For the 1000s graphql handler, the cost is significantly higher. (about 1000 - 1200ms).

Just caching the parseGraphql part gets that down to about 375ms (so we're cutting down to 1/3 of the original cost) on the worst case (highly unscientific test runs). I'll prod at this a little more (likely this weekend) and see where we're spending that time.

[kettanaito]

To be clear: do you refer to the cookie parsing caching as something "generic"? Or the overall idea of putting cache into the handler class?

I think we don't lose anything by doing the latter. In fact, we win a bit by keeping the existing parsing intact (it's complex as it is). It's the upper layer that manages caching anyway, feels right to address it there.

[mattcosta7]

If the Cache is in the Handler class, for instance, we lose the ability to re-use a result between handlers.

all GraphQLHandlers call parse. parse does 2 things - 1, get data for this handler. 1, get general data that only relies on the request object itself.

all HttpHandlers call parse. parse does 2 things. 1 get data for this handler. 2, get general data that only relies on the request object itself.

The shape of the ParsedResult for each of these is different, so for each Request the cache needs an entry for each Handler class but we also can't store just the ParsedResult because what we actually want is just a portion of the parsed result to be cached.

Raising the cache is what I was calling more generic. IE - the cache is no longer specific to a single call signature and shareable across callsites, but global to a run of handlers.

// parsed result is _any handlers definition of it_ which can't be typesafe because users can make their own handlers
const cache = {}
for (const handler of handlers) {
    if (!cache[handler.constructor]) cache[handler.constructor = {value: Symbol('empty') } // using a symbol since undefined is currently a realistic value.
    const parsedResult = handler.run({ request, cacheForHandler: cache[handler.constructor   })
}

extends HttpHandler {
   parse(...) {
       let cacheablePortion: Omit<ParseResult, 'match'>
       if (cacheForHandler.value === Symbol('empty')) {
            const cacheableParsedResult = ......
            cacheForHandler.value = cacheableParsedResult
            cacheablePortion = cacheableParsedResult
       } else {
          cacheablePortion = cacheForHandler.value
       }
       return {
           match: matchUrl(...)
           ...cacheablePortion
       }
   }
}

this is super high level, mostly to show why this is a bit rough to make more generic.
no good way to control what goes into the cache, or separate what should vs shouldn't be cached.

I brought up cookie parsing, because it's the only cacheable portion of the HttpHandler parsing:

msw/src/core/handlers/HttpHandler.ts

Lines 109 to 125 in b440f12

	async parse(args: {
	request: Request
	resolutionContext?: ResponseResolutionContext
	}) {
	const url = new URL(args.request.url)
	const match = matchRequestUrl(
	url,
	this.info.path,
	args.resolutionContext?.baseUrl,
	)
	const cookies = getAllRequestCookies(args.request)
	return {
	match,
	cookies,
	}
	}

Where the only cacheable portion of GraphQLHandler parsing is the parseGraphQLRequest()

msw/src/core/handlers/GraphQLHandler.ts

Lines 136 to 162 in b440f12

	async parse(args: { request: Request }): Promise<GraphQLRequestParsedResult> {
	/**
	* If the request doesn't match a specified endpoint, there's no
	* need to parse it since there's no case where we would handle this
	*/
	const match = matchRequestUrl(new URL(args.request.url), this.endpoint)
	if (!match.matches) return { match }
	const parsedResult = await parseGraphQLRequest(args.request).catch(
	(error) => {
	console.error(error)
	return undefined
	},
	)
	if (typeof parsedResult === 'undefined') {
	return { match }
	}
	return {
	match,
	query: parsedResult.query,
	operationType: parsedResult.operationType,
	operationName: parsedResult.operationName,
	variables: parsedResult.variables,
	}
	}

These subsets are cacheable because they do not change across calls for a single request, whereas url matching do change.

We could break this up a bit such that there's a Handler method call for things that are cacheable an things that aren't (ignore naming)

parse(...) {
   const match = this.matchUrl(request, resolutionContext)
   const parsedRequest = this.parseRequestData(args.request)
   return {match, ...parsedRequest}
}

matchUrl(request, resolutionContext)

parseRequestData = cached(function (request) {
     ...parseRequest.  Access of `this` shouldn't happen here since all data must be from the request only to be cacheable
}

but this extends the api in a not-super intuitive way, and one that's quite brittle, since nothing stops users from accessing this inside of parseRequestData. We could make this a static member, so that this can't be accessed, but static members can't access class generics directly (since the generic is only applicable to the constructor, which statics don't access.

I'd instead opt to avoid trying to make this cache something implemented for all requests, and initially let that be a userland concern. If something is slow, they can cache it as they need to, vs us trying to manage it separately.

[kettanaito]

@mattcosta7, apologies if I communicated it poorly. The intention is to introduce cache to the Network class, which is like a handler orchestrator. The cache can then be shared between handlers.

[kettanaito]

We can address the fact that each handler has its own parsed result in cache by using cache keys similar to what Service Worker API does:

class HttpHandler extends RequestHandler {
  parse({ request }) {
    const cache = this.cache.key('http')
    if (cache.has(request)) {
      return cache.get(request)
    }

    const parsedResult = stuff()
    cache.set(request, parsedResult)
    return parsedResult
  }
}

Depending on whether each handler can actually have a dramatically different caching behavior than this, we may push this to the RequestHandler class and have each individual handler implementation has their own cacheKey: string property set for the cache key.

In a strictly more OOP way, the base class can have an abstract cache() method that each individual handler class implements or not.

cache({ request, parsedResult }) {
  // Decide how, if it all, cache this request.
}

This is really a discussion of intentions:

1. Can we assume all handlers would want to cache their parsing results? Why? Why not?
2. Is there a scenario when a handler would want to opt-out from caching?

Based on the expectations here, we shape the API. Also, we are free to make these assumptions and keep whichever caching behavior we implement as internal. This will make it easier to change it in the future.