mswjs / msw

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

Silent console warning logs from MSW #419

Closed dagadbm closed 4 years ago

dagadbm commented 4 years ago

Is your feature request related to a problem? Please describe. The way I have implemented my mocking solutions implies redundant handlers with a similar url. hence I keep getting bombarded with console warning everytime I do a request.

Describe the solution you'd like I would like a configuration option to be available on the setupWorker (also for node might as well for completion sake) that allows you to omit warnings, if you want to be more picky about this omit several type of warning

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context This is the message in question

Found a redundant usage of query parameters in the request handler URL for "POST bla". Please match against a path instead, and access query parameters in the response resolver function:

dagadbm commented 4 years ago

I have just noticed you also have the quiet option albeit this makes everything too quiet sometimes.. I'll leave it up to you guys to decide what you think is a sane approach to this.

I looked at the code and there are only 3 warning in the code either way, one of them could possibly be even an error (related to the resolver logic), the other one is the onUnhandledRequest message so that one is also out of the picture.

Does it even make sense to have that warning showup? You end up doing what you say you dont do on your documentation: enforce users how they should handle their mock server

kettanaito commented 4 years ago

Hey, @dagadbm. Thanks for reaching out with this.

Are you speaking about the unnecessary query parameter warnings? If so, the warning is valid and you should remove query parameters from the URLs of your request handlers—including query parameters does absolutely nothing and it's a redundant strings in the end. We highlight this in the URL matching section of the docs.

The warning you receive has an intention of signaling "hey, you are doing something that's not necessary", so it has a place to be. Removing query parameters will resolve the warning and make your request handlers much better to read:

- rest.get('/user?id=123', ...)
+ rest.get('/user', ...)

Let me know if there are any other warnings that you experience, or any context that may be important in addressing this issue.

Why should I remove query parameters from a request handler URL? A: Query parameters are not a URL-restricting information, it's a form of an additional request data. That data can still be accessed, but it doesn't define the capturing logic. The warning also guards from false expectations that providing /user?id=123 will not match /user?id=abc or /user?foo=bar, while it actually would. Nevertheless, you can still handle a request based on its query parameters, if necessary, by returning a conditional response.

You end up doing what you say you don't do on your documentation: enforce users how they should handle their mock server

This warning doesn't enforce anything, thus the nature of the message is "warning". MSW is a development tool, and it's alright for such tool to produce warnings if it spots an unintended or unnecessary usage of its API. You can keep the query parameters and nothing will break: warning is not a sign of error. I hope that following the warning's recommendations will both remove it and make your code better.

dagadbm commented 4 years ago

Like I said this depends on how you are implementing your mocks.

For example I am using this with Vue, and per each module of the vuex store there is an endpoints object with the urls per each API, and some of them do indeed depend on the query string to be different.

So something like the image below basically:

The left part is the jobEndpoints object and the right part is the jobHandlers. That apiRequestUrl is a common method that is used to append the host of the backend API and I want this to be as similar as possible to the current code so that I can also use this in testing.

Still I have managed to work around this with the quiet flag but here you find an example of how I am using MSW from a different way that the tool might expect.

dagadbm commented 4 years ago

image

kettanaito commented 4 years ago

Thanks for sharing the details on your usage!

The issue here is that you are transferring a client-side URL mapping into request handlers, which represent server handling of your requests. On a regular server you don't declare routes like /user?id=XXX for multiple reasons:

Taking one of your routes as an example, on the actual server you would do something like this:

// Notice how the request route is clean
app.get('/api/v1/newtranslatortask', (req, res) => {
  const { device, task_type } = req.query
  // respond depending on the query parameters
})

When working with MSW, and its request handlers specifically, you are describing how to respond to requests from a server's perspective. That is why you also never include query parameters in your request handlers URL. Instead, you handle a captured request similar to how an actual server does that:

rest.get('/api/v1/newtranslatortask', (req, res, ctx) => {
  const device = req.url.searchParams.get('device')
  const task_type = req.url.searchParams.get('task_type')
  // respond with a mocked response
})

In fact, the way you have request handlers created for training.get and paid.get is incorrect, as when their query parameters are stripped off their URLs are identical: /api/v1/newtranslatortask. This means that MSW will respond to either of those requests using the *first handler you create in your logic.

Those requests shouldn't be split into two different domains at the first place: they describe the same resource with a different parameters. It looks like training and paid do not reflect the API underneath, but are some sort of high-level abstraction with the API being only a part of it.

I hope this explanation make it clear for you why including query parameters in request handler URLs isn't a good idea. That being said, I understand your concern with a lot of warnings, so I'd like to suggest something to solve that.

Suggestions

Strip off query parameters as a part of apiRequestUrl

The most basic step you can do is to get rid of query parameters in your transformer function.

function apiRequestUrl(url) {
  const urlRecord = new URL(url)
  return urlRecord.href
}

This will also remove hash, which also has no affect on the request matching.

This is not the best solution, however, as it would still leave you with 2 identical request handlers for 2 different request (see my explanation on training.get and paid.get above).

Use conditional response

The right way to approach this is to base your mocked response based on the query parameters within a request handler.

rest.get('/api/v1/newtranslatortask', (req, res, ctx) => {
  const device = req.url.searchParams.get('device')
  const task_type = req.url.searchParams.get('task_type')

  if (device === 'mobile' && task_type === 'paid') {
    return res(ctx.text('ok'))
  }

  // handle other scenarios too
})

This way your request handlers resemble the actual server, introducing limitations that will keep your mocking logic strictly scoped in terms of behavior. You can group such request by introducing a transformation layer between your data structure and request handlers.

Generally, I'd advise to be careful with introducing too much automatization to your mocks. Deriving mocks from some sort of manifesto may get troublesome in the long run: an actual request does not map 1-1 to its request handler. A request handler is like a function declaration, while each individual request is a function call.

dagadbm commented 4 years ago

I think I will indeed need to change my mocks unfortunately because of what you said:

In fact, the way you have request handlers created for training.get and paid.get is incorrect, as when their query parameters are stripped off their URLs are identical: /api/v1/newtranslatortask. This means that MSW will respond to either of those requests using the *first handler you create in your logic.

I guess I will need to spend some time refactoring it and drop my original idea of making this an almost exact replica of how the front end calls it, and yes the api is also probably not the most restful one (they never are)

Also I ended up using your solution instead of mirageJS because it was a bit overkill for me, I just want to send huge ugly json payloads!

kettanaito commented 4 years ago

I'm sure that refactor would be beneficial in the end. Don't hesitate to reach out in case of questions or issues.