mswjs / msw

Industry standard API mocking for JavaScript.
https://mswjs.io
MIT License
15.87k stars 516 forks source link

Add support to intercept "navigate" mode requests on mockServiceWorker #1594

Open prxg22 opened 1 year ago

prxg22 commented 1 year ago

Scope

Adds a new behavior

Compatibility

Feature description

I am trying to mock a HTML form submission using MSW, but it seems that MSW doesn't intercept "navigate" mode requests due to the verification process on the mockServiceWorker. I have already commented out the verification process, but it still doesn't work as it fails in the next verification, cause there's no active client when the form is submitted.

Suggestion:

I suggest adding support for intercepting "navigate" mode requests on mockServiceWorker, which will allow users to fully utilize this feature and mock form submissions with ease.

Impact:

This feature will benefit users who rely on MSW to test their web applications by allowing them to mock form submissions without any restrictions. It will also help users to write better and more reliable tests by providing a comprehensive mocking solution.

kettanaito commented 1 year ago

Hey, @prxg22. Thanks for proposing this.

A bit of history, navigation requests were explicitly blacklisted in the worker we ship in order to prevent situations when you mock your index.html or main.js and break the website entirely until that mock is removed (which, you can imagine, would be rather hard to debug given you have no visual cues on what's going on). I think it's fair to say you never want to mock the main static assets of your application.

The form submission use case is interesting. Can you confirm that a form submission actually arrives at the worker as a fetch event?

prxg22 commented 1 year ago

Yes it does, but the request type is navigate, no matter the form's method. I need to mock a GET request made by a form, but I already tried other ones.

prxg22 commented 1 year ago

Hey, @prxg22. Thanks for proposing this.

A bit of history, navigation requests were explicitly blacklisted in the worker we ship in order to prevent situations when you mock your index.html or main.js and break the website entirely until that mock is removed (which, you can imagine, would be rather hard to debug given you have no visual cues on what's going on). I think it's fair to say you never want to mock the main static assets of your application.

Regarding this issue of blacklisted assets, I had an idea that could help. What if you added another option to the mockServiceWorker that allows users to specify which paths can't be mocked? To use this option, users could call a method setAssetPaths on the worker instance after it's been created, like so:

const worker = setupWorker(...handlers)
worker.setAssetPaths(['http://localhost:3000/foo', '/static/bar/*.js'])

Since this is a breaking change, instead it could recieve an object that configures the asset paths on the setupWorker method. It could also log a warning if the user does not configure the assets, and potentially throw an error if the service worker is initialized with a handler that includes one or more asset paths.

kettanaito commented 1 year ago

Thanks for confirming how form transitions bubble to the worker!

What if you added another option to the mockServiceWorker that allows users to specify which paths can't be mocked?

This doesn't align well with our design principle that the only place that controls request interception is request handlers. In other words, the only way to whitelist/blacklist requests must also be done through the handlers. And, actually, it's already possible with the current API:

// handlers.js
export const handlers = [
  rest.get('/assets/*', (req) => req.passthrough())
]

Including this whitelist handler in the base handlers will mean that even if you attempt to mock a request to /assets, your runtime handlers will simply do nothing.

So, it seems that noting should be blocking us to support navigation requests. My next question would be this: if you write the following handler

rest.post('/login', resolver)

Do you expect this to affect form submissions? We need to think carefully about users' expectations since MSW is primarily for asynchronous resources. For example, I'd not expect the handler above to affect form submissions, I'd expect it to affect fetch('/login', { method: 'POST' }) requests instead.

I'm also curious whether POST form submissions arrive at the worker. You cannot do a POST request in the browser, and that won't be navigation either. Does that work for you? Is event.request.method` really "POST"?

prxg22 commented 1 year ago

Thank you for explaining the design and for the req.passthrough advice, I was not aware of it.

Regarding the support for navigation requests, it seems that there should be nothing blocking us from supporting them. My next question is about the following handler:

rest.post('/login', resolver)

Would you expect this handler to intercept form submissions as well?

I would expect that this handler would be capable of intercepting any POST request to the specified route, whether it is submitted via a form or by using an asynchronous method. In my use case, I am implementing a third-party sign-in process where the user navigates to the third-party sign-in page and then the server responds back to my server in a callback route for both successful and failed sign-ins. During my testing, I am altering the third-party's URL and I wanted to have control over the handlers to redirect the user to the callback route.

I am also curious if POST form submissions arrive at the worker. Although POST requests cannot be made directly from the browser and this is not a navigation request, I wonder if this works for you. Is the event.request.method really "POST"?

Yes, the POST submissions also arrive at the worker. Initially, I thought that POST forms would have a different request.mode, but it turns out that any request made by a form has the type navigate. Here is the log of the request in the service worker, made by a POST form submission:

{
  body: "",
  bodyUsed: false,
  cache: "reload",
  credentials: "include",
  destination: "document",
  headers: Headers {},
  integrity: "",
  isHistoryNavigation: false,
  keepalive: false,
  method: "POST",
  mode: "navigate",
  redirect: "manual",
}
kettanaito commented 1 year ago

That sounds promising, @prxg22!

I think we can draft out the changes necessary to add the support for navigation requests to the library. From the top of my head, we need to solve the initial load assets I've mentioned. While the users can whitelist the assets by creating a corresponding handler, I don't want to push that as a default experience everybody has to do.

Alternatively, I'd propose adding a rule that navigation requests to static resources are always bypassed and cannot be affected by MSW in any way. For example, you cannot mock a GET / navigation request because that will replace your index.html.

What do you think about this direction?

prxg22 commented 1 year ago

I am wondering if there is a way to identify requests for static files made by other tags (such as img, link, or script) and bypass them. However, this would only solve part of the problem.

It is unclear for me if bypassing GET / requests would be sufficient, as it does not guarantee that other routes containing static documents would be mocked.

What if the rest API includes a specific method for mocking navigate requests? For example, something like this:

rest.navigate.get(handler)

This would ensure that the user only mocks the necessary routes and provides them with the option to mock even static routes. While I cannot think of any meaningful use cases for this, it would be a cool feature to have.

kettanaito commented 1 year ago

I'm not a fan of rest.navigate.get(), to be frank. I agree with your statement that specifying a rest.get() handler you expect it to capture all respective GET requests. With the only correction, that image/video/iframe sources do not propagate to the Service Worker, so they won't be affected.

I would pretty much like not to ship any implicit behaviors at all. So that when people mock a GET request to their root-level index.html they know what they are doing. They can also disable MSW at any point in time, and the site would load as usual. Maybe that's the best default experience.

With the upcoming #1436, it will be easier to distinguish between navigation and non-navigation requests because you will get the entire Request instance in your response resolver so you could check request.mode === 'navigate' and then have conditional mocking around that.

kettanaito commented 1 year ago

Quick summary

  1. This is a great proposal and I think we should support navigate requests.
  2. We are still to decide on sensible defaults in regard to the library's design principles.
  3. We are likely to ship this after #1436 because the latter is my main focus right now and I don't want to backport major features such as this one, I simply don't have the strength for that.
prxg22 commented 1 year ago

I'm not a fan of rest.navigate.get(), to be frank. I agree with your statement that specifying a rest.get() handler you expect it to capture all respective GET requests. With the only correction, that image/video/iframe sources do not propagate to the Service Worker, so they won't be affected.

That's a fair point, @kettanaito! So if I understand correctly, you won't disallow any routes from being mocked, right? If that's the case, I agree that this is the best solution.

Before the release of #1436, I would be happy to help accelerate this solution in any way possible. Although I'm not familiar with the specific implementation details of msw, I have recently been working with mockedServiceWorker, which I believe is the main code affected by this solution. However, I still don't fully understand how client activation works, more specifically who sends the postMessage that triggers it.

If you think I could assist in any way, please let me know.

kettanaito commented 1 year ago

Interestingly enough, I was playing around with the request interception in Remix actions (those do regular POST submits of a form), and even the current version of MSW is capable of intercepting what is a navigate POST request. Peculiar.

So if I understand correctly, you won't disallow any routes from being mocked, right? If that's the case, I agree that this is the best solution.

I'd probably just stick to shipping WYSIWYG behavior. You write the handlers, you are responsible if you mock an entry point to your application by mistake. Luckily, it won't be as difficult to revert. I will always choose the path that doesn't introduce implicit behaviors.

I would be happy to help accelerate this solution in any way possible.

Thanks, your help is really appreciated! This feature in particular would require changes to the mockServiceWorker.js script distributed alongside the library, which is a breaking change. I would love not to do any breaking changes before #1436 so I don't lose my mind backporting everything. In that regard, I'm afraid this feature is on hold until the aforementioned pull request lands.

That being said, I like to encourage the philosophy that the third-party code you use is still your code. The mockServiceWorker.js script is no exception. You are free to modify it to unblock the interception of navigation requests and keep using the modified version.

prxg22 commented 1 year ago

Interestingly enough, I was playing around with the request interception in Remix actions (those do regular POST submits of a form), and even the current version of MSW is capable of intercepting what is a navigate POST request. Peculiar.

Remix's Form abstracts the html form element, when the browser has Javascript enabled, Remix will prevent the form to be submitted and use the fetch API to make the request instead. As a result, the requests mode is likely to be other than navigate.

Thanks, your help is really appreciated! This feature in particular would require changes to the mockServiceWorker.js script distributed alongside the library, which is a breaking change. I would love not to do any breaking changes before #1436 so I don't lose my mind backporting everything. In that regard, I'm afraid this feature is on hold until the aforementioned pull request lands.

That being said, I like to encourage the philosophy that the third-party code you use is still your code. The mockServiceWorker.js script is no exception. You are free to modify it to unblock the interception of navigation requests and keep using the modified version.

Thank you for providing further explanation. I understand your concerns regarding the migration of this feature in the future. I appreciate your suggestion and will follow it by experimenting with mockServiceWorker.js. I will keep you informed of any progress I make.

casba commented 1 year ago

I was looking into this too and it doesn't seem to be possible to support responding to navigate requests by modifying the service worker alone. The issue is that the client becomes disconnected due to the beforeunload handler registered during createStartHandler. If you update the service worker to respond to navigate events, and comment out the beforeunload handler, full navigation requests can be intercepted - not that just removing this seems reasonable. Without that event the service worker will store client ids that have disconnected. If I understand correctly the activeClientIds set is used to distinguish connected clients from connected clients that have actually started mocking?

kettanaito commented 1 year ago

@casba, that's an interesting observation. I am tempted to say that behavior shouldn't affect the capture of the navigation requests but I'd like to confirm that before stating anything.

If I understand correctly the activeClientIds set is used to distinguish connected clients from connected clients that have actually started mocking?

Let me tell you a bit more about the activeClientIds and what they're for.

The intention here is to have a self-unregistering service worker. Since workers persist in the browser, it's fairly easy for 1 app at port 3000 to use MSW and then open another, unrelated app at port 3000 the day after and see MSW still working there despite you not even using it in that project.

To tackle this, the worker keeps track of all the (active) clients it controls and whenever those clients reach 0, the worker unregisters itself:

https://github.com/mswjs/msw/blob/c32241627da09af8b34c4068cc17dd1941ff3b0d/src/mockServiceWorker.js#L77-L80

This means that whenever you close the last tab of your app that's using MSW, the worker will automatically unregister itself. If you decide to reopen that tab, your JavaScript code will execute, calling worker.start() again, and registering the worker anew.


So, if I understand your observation correctly, what happens is this:

  1. The navigate requests imply page reload so page reloads.
  2. If that's the only active page (client), this will trigger the worker to unregister (the beforeunload logic I've described above).
  3. Upon the page finishing loading (i.e. navigation request done), the worker won't be registered so it cannot capture that navigate request.

Is that what you've found out?