search

ctimmerm / axios-mock-adapter

Axios adapter that allows to easily mock requests
MIT License
3.44k stars 244 forks source link

Reconsider order of operations with regards to replyOnce #257

Open slightlytyler opened 4 years ago

slightlytyler commented 4 years ago

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

ctimmerm commented 4 years ago

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

slightlytyler commented 4 years ago

@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());
})
ctimmerm commented 4 years ago

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.

slightlytyler commented 4 years ago

Happy to implement it as an additional method 👍

sanpoChew commented 4 years ago

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

slightlytyler commented 4 years ago

@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

jueinin commented 3 years ago

any progress on this override topic?