msw icon indicating copy to clipboard operation
msw copied to clipboard

Provide a way (or document an existing way) to return a NetworkError once

Open noahbrenner opened this issue 4 years ago • 10 comments

Is your feature request related to a problem? Please describe.

I would like to have a documented way of defining a handler that returns a NetworkError only once, then falls back to an existing handler. This can be helpful in testing an app's request-retrying logic.


I know how to return a valid response or a failing status code once:

server.use(
  rest.get('/foo', (_req, res, ctx) => {
    return res.once(
      ctx.status(404, 'Not found')
    );
  })
);

And I know how to return a network error:

server.use(
  rest.get('/foo', (_req, res, ctx) => {
    return res.networkError('Failed to conect');
  })
);

But since these both depend on returning the result of different methods on res, I'm not sure how to use the current API to return a network error once.


I tried a few options, including the three below, but they all continued to respond with errors after the first request. They also resulted in TypeScript errors, which makes sense:

server.use(
  rest.get('/foo', (_req, res, ctx) => {
    return res.once(res.networkError('Failed to conect'));
  })
);
server.use(
  rest.get('/foo', (_req, res, ctx) => {
    return res.once(() => res.networkError('Failed to conect'));
  })
);
server.use(
  rest.get('/foo', (_req, res, ctx) => {
    return res.once((innerRes) => innerRes.networkError('Failed to conect'));
  })
);

Describe the solution you'd like

I'm not familiar with msw's internals, so there may be a better fit for the API, but an intuitive solution for me would be:

  • Provide networkError on the ctx object. This would be consistent with the API for changing just about every other aspect of a response (networkError strikes me as out of place on the res object).

It could then be used something like this:

server.use(
  rest.get('/foo', (_req, res, ctx) => {
    return res.once(
      ctx.networkError('Failed to conect')
    );
  })
);

Describe alternatives you've considered

Another good option would be to provide networkError as a method on once:

server.use(
  rest.get('/foo', (_req, res, ctx) => {
    return res.once.networkError('Failed to conect');
  })
);

Or to provide once on rest and graphql and all request methods on once:

server.use(
  rest.once.get('/foo', (_req, res, ctx) => {
    return res.networkError('Failed to conect');
  })
);

Or to provide once as a method of all request methods and have it inherit the preceding method's behavior:

server.use(
  rest.get.once('/foo', (_req, res, ctx) => {
    return res.networkError('Failed to conect');
  })
);

Additional context

I'm guessing there may already be a way of accomplishing this, just requiring digging into the internals. If so, it would be wonderful to add documentation for it. This would help users and discourage them from relying on undocumented portions of the API that might change.

noahbrenner avatar Oct 03 '20 00:10 noahbrenner

I might be wrong but I think networkError can't be used with once currently. As a work around you might want to use the status to create a falsy response which can be used with once, see https://mswjs.io/docs/recipes/mocking-error-responses for more info.

timdeschryver avatar Oct 03 '20 08:10 timdeschryver

Thanks for the response, @timdeschryver. I'm also testing error responses (in my case 500), but since there's a difference between receiving an error response and receiving no response, that doesn't cover testing the latter use case.

Was there something on that doc page I missed? I'm not seeing anything about falsy responses, just responses with status codes other than 2xx/3xx.

noahbrenner avatar Oct 09 '20 03:10 noahbrenner

Hey, @noahbrenner. You're right, an error response (i.e. 500) and the network error (no response) are two substantially different cases.

There is no API on res to support both res.once and res.networkError at the moment. However, you can still respond with a network error once by utilizing runtime handlers.

const server = setupServer(...)

afterEach(() => {
  // This removes any handlers you add with ".use()" in each test.
  server.resetHandlers()
})

it('handles a network error', () => {
  server.use(
    rest.get('/user', (req, res, ctx) => res.networkError())
  )
})

Although there's no res.once that marks itself done once hit, utilizing resetHandlers and use produces a similar result. Note the difference that in the example above we reset the handlers in the afterEach hook. This means that all your requests to /user as a part of that test will produce a network error, not just the first one.

If you wish to have a more granular control over when the server should stop using your runtime handler, you can call server.resetHandlers() within your test suite.

kettanaito avatar Oct 09 '20 09:10 kettanaito

As for res.once and res.networkError, I find it an API design flaw to have them both on the res function. res.once augments the response behavior, so it's okay that it's replacing the regular res (cannot combine the two). The network error, however, is a way to configure the response itself (not the internal representation of it) and putting it on the res function reads off to me.

This brings the question of where to include networkError.

Create a new context utility

Context utilities are designed to be composable. Network error cannot be composed with any other context utilities: a network error cannot have status or headers (on the other hand it could be delayed).

Validate context utilities integrity

Alternatively, the library could validate if the context utilities you've provided make sense together. This would make such usage work:

res(ctx.delay(1000), ctx.networkError('Oops'))

while these would produce a warning on impossible utilities combination:

res(ctx.status(200), ctx.networkError())

A huge drawback of the validation is its resource cost and overall increase of complexity to the internals, which I'd rather avoid.

Introduce something entirely new

As it doesn't belong to either res nor ctx, perhaps it's a call to create an entirely new API for the network error. The drawback for this is the lack of other similar APIs we could group as a part of this new one. A network error utility alone doesn't sound like a sufficient reason to introduce an entirely new API concept to the library.

I'm open to your suggestions on this topic: how do you see such API working?

kettanaito avatar Oct 09 '20 09:10 kettanaito

Hi @kettanaito! Runtime handlers are really great (you can see me using them in the original post), but they don't quite fit my use case. I'm testing behavior that retries a request automatically if it fails, so there's only one call to my API that results in both the first (failed) request, and the retry. That means there's no place to put a call to server.resetHandlers() between those two attempts.


You make good points about networkError not quite fitting on the ctx object. I've been brainstorming a bit today, and here are a couple ideas I came up with for a new API:

Option 1

The NetworkError class could be exposed directly, in place of the .networkError() method that uses it. The error could be thrown inside a raw res() callback to trigger a network error response.

Usage could look like this:

import { rest, NetworkError } from "msw";
...
rest.get("/foo", (res) => {
  res.once(() => {
    throw new NetworkError("Whoopsie!");
  });
});

Implementing this would require...

  • res.once() to accept a callback the same way res() does (I think that's already the case, but I haven't double checked).
  • Both res() and res.once() to use a try...catch block to catch the error.
    • When catching the error, res.once() would need to mark the response as "once" to distinguish it from the alternative.

Benefits

  • It's intuitive that a thrown error supersedes a return statement, since return will never be reached. So it isn't surprising that the response is not actually delivered in this example:
    res.once((resInner) => {
      throw new NetworkError("Whoopsie!");
      return resInner;
    });
    
  • It's versatile; you could use conditions if you wanted to:
    res.once((resInner) => {
      if (someCondition) {
        resInner.status = 200;
        ...
        return resInner;
      } else {
        throw new NetworkError("Whoopsie!");
      }
    });
    

Option 2

.networkError() could be provided on res.once in addition to where it's currently provided on res. This could look like:

rest.get("/foo", (res) => {
  res.once.networkError("Whoopsie!");
});

There would need to be some way to distinguish between calls to res.networkError() and res.once.networkError(). Currently, the method just throws an instance of NetworkError, so the implementation would need to change somehow or other to allow the rest.get() handler to tell which is which.

Benefits

  • This wouldn't be a breaking change, since res.networkError() would behave the same way (even if implemented differently), and a major version bump would not be necessary.
  • It's less verbose than Option 1, though also less versatile.

Other thoughts

  • With either of these options, res and res.once would have the same API, making them more intuitive to use. A developer could change any res(...) to a res.once(...) without wondering which features are supported by each one.

  • Neither of these options account for adding a delay, though I suppose either one could accept a delay as a parameter, either raw or in a config object.

  • I suggest making the argument to networkError()/new NetworkError() optional by adding a default parameter value (so we'd call networkError() rather than networkError("foo")). From what I've seen, the message passed in is never returned in a way accessible to consumers of the package, so it just feels like clutter in my code. The only reason I don't recommend getting rid of the argument altogether is for backward compatibility (JavaScript wouldn't complain about passing an unused argument, but TypeScript would).

noahbrenner avatar Oct 10 '20 03:10 noahbrenner

I found a workaround for now, though I'd still love to see this supported natively. Here's what's working for me:

function enableSuccessfulResponse() {
  server.use(
    rest.get("http://example.com", (_req, res, ctx) => {
      return res(ctx.json({ foo: "bar" }));
    })
  );
}

server.use(
  rest.get("http://example.com", (_req, res) => {
    enableSuccessfulResponse();
    return res.networkError("Failed to connect");
  })
);

noahbrenner avatar Dec 24 '20 04:12 noahbrenner

having .once chained on either rest, res or ctx feels a bit odd to me as it's not immediately obvious if it's once per test or once per call to that route

edit: I hadn't come across res.once before in the API so thought this was a new suggestion - I guess none of the below thoughts are strictly networkError related

const networkErrorHandler = rest.get("/foo", (res) => {
  res.once.networkError("Whoopsie!");
});

server.use(networkErrorHandler)

afterEach(() => {
  server.resetHandlers()
})

it("first test", () => {
  doFetch() // does this error
  doFetch() // and this pass
})

it("second test", () => {
  doFetch() // does this error or inherit the pass?
})

It feels like logically it would be once per test?

Another API that would feel more in-line with how I'm used to mocking (with jest) would be something like server.useOnce

const networkErrorHandler = rest.get("/foo", (res) => {
  res.networkError("Whoopsie!");
});

afterEach(() => {
  server.resetHandlers()
})

it("first test", () => {
  server.useOnce(networkErrorHandler)

  doFetch() // errors
  doFetch() // default handler behaviour
})

it("second test", () => {
  doFetch() // default hanlder behaviour
})

it("always fails", () => {
  // networkErrorHandler concerns are separated, it's not bothered about how many times it's called
  server.use(networkErrorHandler)
  
  doFetch() // errors
  doFetch() // errors
})

mcky avatar Nov 01 '21 12:11 mcky

Hey, @mcky.

The .once chain means once per hit. Effectively, it's a self-deactivating request handler that will only process the matching request once. After that, it will be marked as skipped internally and will not affect the traffic anymore. Based on that, the current behavior of .once is what you want for .useOnce.

As the number of response scenarios grows, I agree that res.once() may not be the best API to utilize. I'm somewhat reluctant to consider this chaining for rest.once and graphql.once, which ruins the semantics of those handlers, specifically them resembling the resource they are capturing. I'm also against any kind of configuration objects as additional arguments to request handlers.

Historically, res.once was chosen as a special API that reads as "use this response only once". That is, however, not entirely true in terms of how the handler behaves. It's the entire handler that's used once, not the particular response. Logic-wise, the .once modified should be declared somewhere on the request handler's level but I haven't found a suitable API for that as of now. I'd love to hear more opinions on this.

kettanaito avatar Nov 08 '21 01:11 kettanaito

How about providing a token/constant?

server.use(
  rest.get('/foo', (_req, res, ctx) => {
    return res.once(NETWORK_ERROR);
  })
);

or an error without throwing?

server.use(
  rest.get('/foo', (_req, res, ctx) => {
    return res.once(new NetworkError());
  })
);

res and res.once already accept null and undefined and will return a 200 response with an empty body.

server.use(
  rest.get('/foo', (_req, res, ctx) => {
    return res.once(); // I would assume the server returns nothing, not even 200 (aka a network error)
  })
);

If it is okay for the values null and undefined to be mapped to a specific behaviour, then it should be okay to add another such value e.g. new NetworkError() and map it to a different behaviour.

MartinJaskulla avatar Apr 05 '22 13:04 MartinJaskulla

I'm considering the introduction of "intentions" returned from the resolver (see #1207). Returning a mocked response object is no longer flexible as the scenarios for mocks grow in complexity. I believe the intention-driven design will allow us to make resolvers more straightforward. It can also allow us to decouple intentions ("return network errors") from modifiers (always/once), which should fix this particular issue.

kettanaito avatar May 02 '22 10:05 kettanaito

Released: v2.0.0 🎉

This has been released in v2.0.0!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

kettanaito avatar Oct 23 '23 08:10 kettanaito