Make request handling synchronous by default

[danny-andrews]

Thanks for making this library! Very simple and easy to use. However, there is one aspect of it that makes testing a little more difficult than it needs to be. That is the fact that every handler is shunted through a setTimeout function. This keeps tests from being able to be written in a synchronous fashion and ended up requiring me to do the following:

it('request sets correct headers', () => {
  const requestSpy = jest.fn((req, res) => res.status(200).body('{}'));
  mock.mock(requestSpy);

  subject();

  return new Promise(resolve => setTimeout(resolve, 0)).then(() => {
    const actual = requestSpy.mock.calls[0][0].headers();
    expect(actual).toEqual({
      accept: 'application/json',
    });
  });
});

With the changes in this PR, I can write the same test without the Promise/setTimeout funny-business:

it('request sets correct headers', () => {
  const requestSpy = jest.fn((req, res) => res.status(200).body('{}'));
  mock.mock(requestSpy);

  subject();

  const actual = requestSpy.mock.calls[0][0].headers();
  expect(actual).toEqual({
    accept: 'application/json',
  });
});

[danny-andrews]

@jameslnewell

[jameslnewell]

Hi @danny-andrews and thanks for the PR!

Since the real XHR is always asynchronous I think its probably better to keep the mock XHR asynchronous too in order to avoid hitting edge-cases and bugs in prod that might not occur in test due to the difference in async vs sync.

I think the easiest way to get around this is having your subject() use a callback or a Promise e.g.

it('request sets correct headers', () => {
  const requestSpy = jest.fn((req, res) => res.status(200).body('{}'));
  mock.mock(requestSpy);

  return subject().then(() => {
    const actual = requestSpy.mock.calls[0][0].headers();
    expect(actual).toEqual({
      accept: 'application/json',
    });
  });
});

I understand that will sometimes make tests more difficult but think its worth catching those edge-case bugs that occur sometimes. What do you think?

[danny-andrews]

🤔 I see your point about wanting to make xhr-mock behave as closely as possible to the native implementation, but I don't believe it should make a difference in this case. The way I understand it, with callback-based asynchrony, it shouldn't matter wether the code is actually async or not. It just matters that the callback is called at some point.

Here's an article that explains this concept very well (specifically the Async-compatible code vs. Intrinsically async code part): https://martinfowler.com/articles/asyncJS.html.

(I may be missing an edge-case here, so if you can think of a counter-example, let me know!)

Also, thanks for the workaround. I think I'll use that strategy in the meantime.

[jameslnewell]

Can't remember specific examples but I've definitely run into it in the past with things scheduled on the next "tick". I believe promises are always async to avoid newbies running into the same issue.

One of my TODOs for this lib is "Ability to return mocked responses asynchronously" (in order to allow proxying real requests, custom timeout logic etc) for which I think it'll need to remain async.

How'd you go returning a promise or calling a callback from within your subject()?

[danny-andrews]

Turns out xhr requests aren't always async! However in practice, they are, since many browsers have deprecated it.

Also, I didn't realize Promises were always async! TIL :D

With all this in mind, I agree it's a good idea to keep this library async. Thanks for the discussion! Calling a callback from within my subject() will work just fine.

[jameslnewell]

Aaaah sorry. I didn't realise you were talking about the sync mode, I thought you wanted to return synchronously when in async mode. I've started work on v2 (check the branch) which I'll implement the standard sync mode.

(and I assumed the sync mode could go unimplemented since who would use it 😶)