mswjs / http-middleware

Spawn an HTTP server from your request handlers or apply them to an existing server using a middleware.
https://npm.im/@mswjs/http-middleware
107 stars 13 forks source link

Initial implementation of middleware #1

Closed idolize closed 3 years ago

idolize commented 3 years ago

@kettanaito here is my initial implementation of the middleware (as well as a simple express server in case users just want something batteries-included for serving mocks).

A few issues I noticed while working on this:

  1. (As previously discussed) msw does not expose any way to import getResponse at the moment, so I had to copy/paste it for now
  2. I'm not sure if this changed recently (since I mainly use GraphQL in my application), but I noticed that now the rest mocks fail to match when running in the Node server unless the mock definition includes the full, absolute URL root in the "mask" (e.g. /login fails to match, but http://localhost:9090/login works)
    • I tracked this down to this code, which looks for a global location variable (that doesn't exist in node) to convert any relative mask strings into absolute URL strings
    • So, since there is no location, the URL is not absolute, and then the URL constructor falls back to string. Then we try to compare a relative mask against an absolute request URL, which ultimately fails to match under node-match-path
    • In terms of how to resolve this:
      • Option 1 - Update the Matching Logic: I imagine it should be possible to update this logic slightly to handle relative path masks by passing in some kind of context (which, in the case of node, could be created via the req object) instead of hardcoding the check for the location global
      • Option 2 - Use Relative URL in the Mocked Request: If we relax the url property of MockedRequest to be string instead of URL then we could change the code that constructs the mock request in this PR's middleware.ts file to not pass a the full absolute URL in the case where we know the request is a relative URL (which I believe would also solve the matching issue)
      • There may be other solutions as well, but I'd like to get your thoughts on this

I also haven't added much in the way of unit testing currently (especially given that msw itself is well-tested, and if I mock express then I'm not sure how much we are gaining in confidence) - what do you think would be the best testing strategy for this? Maybe something like this?

And obviously if you have any other feedback on the API design, documentation, examples, etc. that is welcome too.

kettanaito commented 3 years ago

Hey, @idolize. Thank you for preparing the initial implementation!

I've opened a pull request that exports a few functions we can use when matching a request against handlers in the middleware function. I'll update this pull request shortly to use those functions and also introduce a few minor changes.

kettanaito commented 3 years ago

Regarding the relative URLs: there's no limitation. There is no such thing as a relative URL in Node.js (nothing to be relative to), so constructing a URL without the base URL rightfully fails.

MSW never supported relative URLs in Node.js. In most cases when developers use MSW in Node.js they're running tests in a browser-like environment (like JSDOM), where location is polyfilled (Jest sets location.href to http://localhost). This is what MSW leverages when handling relative URLs in Node.js:

https://github.com/mswjs/msw/blob/62ceeebdce5a995f55fc00a60cf50b75e3302d08/src/utils/url/getAbsoluteUrl.ts#L12-L14

In the context of a Node.js server, there will be no location, so a relative request handler URL will be left as-is, resulting in an exception when constructing a MockedRequest for the matching logic.

Suggested solution

What I've done so far is added the logic that goes through each request handler and modifies its info.mask (URL) information, appending the server's origin to each relative request handler URL.

kettanaito commented 3 years ago

Reviewing my suggested solution above, I've realized that it mutates the handlers along the way. This isn't what we should do, so I don't find it appropriate.

Adjusting the matching logic sounds interesting, would love to think about the API that'd allow that. A drawback of that would be a potentially significant internal change to MSW, which would be nice to avoid, if possible.

kettanaito commented 3 years ago

I also haven't added much in the way of unit testing currently (especially given that msw itself is well-tested, and if I mock express then I'm not sure how much we are gaining in confidence) - what do you think would be the best testing strategy for this?

I've added a test for the middleware, where it's being applied to an actual express server and the response to the matching/un-matching routes is being asserted. That's a good start.

idolize commented 3 years ago

@kettanaito I've pushed a change that causes the tests to pass and avoids mutating the original handlers. It works by constructing Proxy objects for all RestHandler instances initially, and then on every subsequent request it calls a special new getter function for info.mask instead of just accessing the field directly.

The above solution could also be simplified slightly to avoid the getter by just creating a copy of every RestHandler on every request instead, but I did it this way to make it more performant (since it only has to update one function each time instead of allocating/garbage collecting all the handlers on each request).

It's a bit of a hack, but I think this may be good enough to get this merged/released for now - unless you want to consider a light API refactor at the msw core library level (perhaps to allow additional context to be passed into the matching methods)

kettanaito commented 3 years ago

Thanks for the effort, @idolize. I'm wrapping up an extension on the handleRequest side that'd support resolutionContext.baseUrl so we could pass it in the middleware without having to care about the underlying URL matching process. I believe there'll be no need in using a proxy.

kettanaito commented 3 years ago

Here's the resolutionContext.baseUrl pull request: https://github.com/mswjs/msw/pull/830

kettanaito commented 3 years ago

@idolize, I'm sorry I had to remove your proxy implementation, but on the bright side, we now have the code library's support in specifying the base URL using the resolutionContext option on handleRequest.

I've also added the reusing-handlers.test.ts test suite to verify that the same handlers can be reused for both middleware and in-process request interception.

idolize commented 3 years ago

Perfect! That's even cleaner

kettanaito commented 3 years ago

@idolize, thank you for collaborating with me on this 🎉

I've published the first version and wrote about it on DEV. I hope you don't mind me mentioning you.

idolize commented 3 years ago

@kettanaito Amazing! Thank you for being receptive to the idea and working with me to get this over the finish line. I know personally it's going to come in very handy (and I hope others can benefit as well!)

kettanaito commented 3 years ago

I'm excited to see people using this! I think embracing challenging use cases through standalone extensions is the right track, and this is a great example.

Let me know if you'd like to collaborate on more things together, I'd be thankful!