Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconsider order of operations with regards to replyOnce #257

Open
slightlytyler opened this issue Apr 7, 2020 · 7 comments
Open

Reconsider order of operations with regards to replyOnce #257

slightlytyler opened this issue Apr 7, 2020 · 7 comments

Comments

@slightlytyler
Copy link

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
Copy link
Owner

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

@slightlytyler
Copy link
Author

@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
Copy link
Owner

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
Copy link
Author

Happy to implement it as an additional method 👍

@sanpoChew
Copy link

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

@slightlytyler
Copy link
Author

slightlytyler commented Jul 29, 2020

@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 mswjs/msw#128

@jueinin
Copy link

jueinin commented Dec 14, 2020

any progress on this override topic?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants