axios-mock-adapter icon indicating copy to clipboard operation
axios-mock-adapter copied to clipboard

Reconsider order of operations with regards to replyOnce

Open slightlytyler opened this issue 5 years ago • 7 comments

Love the library but this one detail is preventing me from adopting it. Consider Jest's implemention of mockImplementationOnce https://jestjs.io/docs/en/mock-function-api.html#mockfnmockimplementationoncefn

const myMockFn = jest
  .fn()
  .mockImplementationOnce(cb => cb(null, true))
  .mockImplementationOnce(cb => cb(null, false));

myMockFn((err, val) => console.log(val)); // true

myMockFn((err, val) => console.log(val)); // false

So far looks very similar to replyOnce right? This next section is where it differs:

const myMockFn = jest
  .fn(() => 'default')
  .mockImplementationOnce(() => 'first call')
  .mockImplementationOnce(() => 'second call');

// 'first call', 'second call', 'default', 'default'
console.log(myMockFn(), myMockFn(), myMockFn(), myMockFn());

With axios-mock-adapter's current order of operations the log would be 'default', 'default', 'default', 'default'. Reason being is that replyOnce and reply are kept in the same list and replyOnce handlers are pushed to the end https://github.com/ctimmerm/axios-mock-adapter/blob/master/src/index.js#L213

Adopting this pattern would be a breaking change and require a bit of reorganization in terms of the internal data structures. If you think this fits in with the project I'd be happy to contribute rather than purse a fork

slightlytyler avatar Apr 07 '20 19:04 slightlytyler

Could you give an example of how you're trying to use axios-mock-adapter?

ctimmerm avatar Apr 13 '20 18:04 ctimmerm

@ctimmerm here's an example of how I would like to use the library in an integration test setting

it('should show an error when API is 500-ing', async () => {
    mockHttpClient.onAny().replyOnce(500);
    const { queryByText } = render(<App />);
    await waitFor(() => expect(queryByText(/error/i)).toBeVisible());
})

slightlytyler avatar Apr 13 '20 21:04 slightlytyler

The problem is that it would be a very large breaking change requiring large effort and having some behavior that can't be migrated.

I think it would work better as an addition to the current API rather than a change, e.g. by introducing a new replyOnceFirst method.

ctimmerm avatar Apr 18 '20 21:04 ctimmerm

Happy to implement it as an additional method 👍

slightlytyler avatar Apr 19 '20 17:04 slightlytyler

@slightlytyler was wondering if you have any plan to open a PR? I see you've published your fork on npm

sanpoChew avatar Jul 29 '20 09:07 sanpoChew

@sanpoChew I'm no longer on the project that needs this so probably not. My fork has changes that match what I originally envisioned which would be a breaking change, not ctimmerm's proposal. I made a similar proposal for a different tool which has landed now so there are other options if you need something like this API right now in an available library https://github.com/mswjs/msw/issues/128

slightlytyler avatar Jul 29 '20 15:07 slightlytyler

any progress on this override topic?

jueinin avatar Dec 14 '20 06:12 jueinin