fix: don't fullfill requests after stop method was called

[mjetek]

• Fixes Error: route.fulfill: Route is already handled! in After hooks #15

Roadmap

• Use page.unroute() (see this)
• Add a failing test

[kettanaito]

Hi!

Since the class extends SetupApi, it can push to the this.subscriptions array to clear up after any side effects. That array will be exhausted in the super.dispose() method, which is called on stop().

playwright/src/index.ts

Line 71 in 0eebdc3

export class NetworkFixture extends SetupApi<LifeCycleEventsMap> {

What we have to do instead here is this.subscriptions.push(cb) that will remove the route handler from this.#page so it stops affecting requests. Since recovering from stop is always start, there route handler will be added back once the instance is restarted.

We should use page.unroute() and probably store the handler function itself so we are able to reference it for routing and unrouting.

What do you think about this, @mjetek?

[mjetek]

Hi! You're right that my initial approach wasn't very elegant. I've updated the PR with your suggested implementation (please let me know if I misunderstood anything).
I need to familiarise myself with how msw itself works.

[kleinfreund]

I'm trying to use @msw/playwright together with msw and playwright in a project and I'm running into #15 as well so I figured I use these changes in that project to try if that fixes the issue.

It largely seems to do so, but I still see a few tests fail with:

Error: route.continue: Route is already handled!

This happens to arbitrary tests that use msw and is not stable. One test that failed on the current run might pass on the next one.

Tests that fail seem to be categorically those that trigger further network requests that are subject to msw mocking toward the end of their test run. In my project, it's exclusively tests for asserting the submission of HTML forms behaves correctly. At the end of those tests, a redirect to the created/updated resource page occurs which in turn loads that resource (i.e. the aforementioned further network requests). So far I'm not able to reproduce this independently in a test case for the @msw/playwright project to work on it in isolation.

@mjetek One thing I noticed is that you're still calling unroute in the stop method. I think that needs to be removed now since dispose will clean up the subscriptions and thus call unroute (however, doing so introduces new errors in the form of Error: browserContext.close: Target page, context or browser has been closed).

That made me wonder: With the subscriptions being populated with asynchronous functions, can there be a race condition with Disposable.prototype.dispose not awaiting the individual subscriptions, @kettanaito?

More notes:

• I noticed that the "Route is already handled!" failures actually start with Internal error: step id not found: fixture@233
• It seems like the frequency of the "Route is already handled!" failure occurring is somewhat higher on Node 23.11.1 compared to 24.8.0
• I can bring down the frequency of the "Route is already handled!" failure considerably by writing my tests in such a way that there is little to no network activity as a test concludes and a subsequent one starts. Consider a test for page that has a form for creating a resource which, after submitting successfully, redirects to a page for the created resource. My tests would typically conclude with a check that the submission was successful and leave testing what happens after the redirect to another test. By adding some basic assertions to the end of such tests to make sure the page redirected to has loaded its content, too (e.g. that a table is visible), the failure rate went down considerably but never to near-0.
• By applying this PR's changes plus my suggestion from #17 (comment) via patch-package and upgrading to Node 24.8.0 and "stabilizing" my tests so they conclude with little to no network activity (something I'd like to avoid, though), I can bring down the failure rate dramatically. That makes me believe this work is on the right track but that there is some sort of race condition in the teardown logic

[kleinfreund]

Suggestion: I was checking out this work locally and found that running the project's test script started to produce the following error with your changes:

    TypeError: Cannot read properties of undefined (reading 'handlersController')

One way to rectify this error is to turn httpHandler into an arrow function:

Suggested change

	async httpHandler(route: Route, request: Request) {
	httpHandler = async (route: Route, request: Request) => {

That will make the tests pass.