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

[Proposal] possible to pass the intercept function to the mswmock handler #1668

Open
soartec-lab opened this issue Oct 22, 2024 · 8 comments
Assignees
Labels
enhancement New feature or request mock Related to mock generation

Comments

@soartec-lab
Copy link
Member

What are the steps to reproduce this issue?

This issue is new proposal

What happens?

This makes it easy to check whether this API is being called correctly in a test, for example.
Currently, to achieve this, it is not possible to use generated mocks, and necessary to implement them in tests each time.
By doing this, it becomes possible to intercept function execution, making testing easier.

import { Ok } from '/schemas'

getGetPetMockHandler({ message: 'hello'})

export const getPutPetMockHandler = (overrideResponse?: Ok | ((info: Parameters<Parameters<typeof http.get>[1]>[0]) => Promise<Ok> | Ok)) ) => {
  http.get('*/pets/:id', async (info) => {
    return new HttpResponse(
      JSON.stringify(
        overrideResponse !== undefined
          ? typeof overrideResponse === 'function'
            ? await overrideResponse(info)
            : overrideResponse
          : getGetPetResponseMock(),
      ),
      {
        status: 200,
        headers: {
          'Content-Type': 'application/json',
        },
      },
    )
  })
}

What were you expecting to happen?

I want to use it like below:

import { Ok } from '/schemas'
import { vi } from '/vitest'

const mockFn = vi.fn()

getGetPetMockHandler(
  {
    overrideResponse: { message: 'hello'},
    intercept: (info: any) => {
      mockFn(info)
    }
  }
)

export const getPutPetMockHandler = (
  {
    overrideResponse,
    intercept,
  }: {
    overrideResponse?: Ok | ((info: Parameters<Parameters<typeof http.get>[1]>[0]) => Promise<Ok> | Ok),
    intercept?: (info: Parameters<Parameters<typeof http.get>[1]>[0]) => void
  }
) =>
  http.get('*/pets/:id', async (info) => {
    if (intercept) {
      intercept(info)
    }

    return new HttpResponse(
      JSON.stringify(
        overrideResponse !== undefined
          ? typeof overrideResponse === 'function'
            ? await overrideResponse(info)
            : overrideResponse
          : getGetPetResponseMock(),
      ),
      {
        status: 200,
        headers: {
          'Content-Type': 'application/json',
        },
      },
    )
  })

Any logs, error output, etc?

none.

Any other comments?

If i make this change, it will be a breaking change.

What versions are you using?

Please execute npx envinfo --system --npmPackages orval,zod,axios,msw,swr,@tanstack/react-query,@tanstack/vue-query,react,vue and paste the results here.

@soartec-lab soartec-lab added the enhancement New feature or request label Oct 22, 2024
@soartec-lab
Copy link
Member Author

Hi, @melloware
What do you think this?

@soartec-lab soartec-lab self-assigned this Oct 22, 2024
@melloware melloware added the mock Related to mock generation label Oct 22, 2024
@melloware
Copy link
Collaborator

Sounds reasonable to me

@severinh
Copy link
Contributor

severinh commented Oct 22, 2024

Thanks for taking the time to write the proposal, @soartec-lab!

I'd like to better understand what you'd like to achieve in the example you give. Couldn't you already achieve what you describe with the current API of orval?

getGetPetMockHandler((info) => {
  // You can do anything you want here, such as asserting that the API is called.
  return { message: 'hello'};
});

What worries me (as you already point out) is that the proposal would be a breaking change that will require developers who use orval to change many/all of their tests. And simple tests (without intercept) would require more boilerplate, such as getGetPetMockHandler({ overrideResponse: {...} }) instead of just getGetPetMockHandler({...}).

@soartec-lab
Copy link
Member Author

soartec-lab commented Oct 22, 2024

@severinh

Let me introduce you to that thinking.
I would like the function to run when the API is called, not if I call the handler function.
Using handler(function) will call function now. However, intercept is executed when the API is called.

@severinh
Copy link
Contributor

severinh commented Oct 24, 2024

Thanks @soartec-lab. I think there's a misunderstanding here.

When you pass a function to getGetPetMockHandler, this actually gets called not immediately, but whenever the API is called. Each time the application makes a request to the example pet endpoint, that function will be called.

getGetPetMockHandler((info) => {
  // This will be called each time a request to the API is made. You can anything you want to do here, such as making assertions.
  // You can for example even access the concrete request that the application made, via `info.request`.
  return { message: 'hello'};
});

@soartec-lab
Copy link
Member Author

soartec-lab commented Oct 24, 2024

@severinh

Exactly. I certainly misunderstood.
I thought this feature existed before, and it seemed strange, but now my misunderstanding has been cleared up.
Thank you let me know 👍

But there is a further problem.
After making this improvement, I was trying to add status and header to the option type of the argument.
To add these, I think it is necessary to introduce an option type that involves breaking changes.
You previously said in #1206 that you also want to override status, but wouldn't you like to be able to write something like this in the end?

getGetPetMockHandler(
  {
    overrideResponse: { message: 'hello'},
    overrideStatus: 500,
    overrideHeader: { 'Content-Type': 'application/pdf' },
    intercept: (info: any) => {
      mockFn(info)
    },
  }
)

@severinh
Copy link
Contributor

severinh commented Nov 1, 2024

@soartec-lab You're right that being able to override more aspects of the HttpResponse could be useful. Right now our team does not have a strong need for this anymore, but others might.

However, I don't think passing an object with all kinds of override* fields that map to HttpResponse properties is a clean approach. You would end up building an unnecessary API clone/mirror of the MSW HttpResponse API. I think it's important that we have a clean separation of responsibilities between MSW and Orval. Orval should complement MSW, not replace/clone MSW APIs, or prevent developers from interacting directly with MSW.

To give developers more control over the MSW HttpResponse, I think the best approach would be for Orval to directly let developers provide that MSW HttpResponse object themselves, if they need to. Something like this:

getGetPetMockHandler((info) => {
  // if the developer wants, they can do anything here, such as inspect info.request
  return HttpResponse.json({ message: 'hello'}, { headers: 500, contentType: ... });
});

That would not require a complex new API - you'd just give Orval developers the option access the MSW API, if they want to. Btw, this is also the approach taken by other MSW integrations (such as for GraphQL).

Most importantly, Orval should keep the option to just use the existing, simple, very compact getGetPetMockHandler({message: 'hello'}). I'm sure that's what 95%+ of Orval developers mainly use, and this should not change. Requiring developers using Orval to replace these calls with getGetPetMockHandler({overrideResponse: {message: 'hello'}}) would not be well received due to the unnecessary boilerplate.

What do you think? I hope that's understandable. Happy to provide more details, but I'll be off the grid for the next 3 weeks.

@soartec-lab
Copy link
Member Author

@severinh

Thank you for your reply.
I can understand what you are saying. This isn't an urgent issue for me, so I'll wait for other people's feedback before considering it.
I think this approach is my make sense for now 👍

getGetPetMockHandler((info) => {
  // if the developer wants, they can do anything here, such as inspect info.request
  return HttpResponse.json({ message: 'hello'}, { headers: 500, contentType: ... });
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request mock Related to mock generation
Projects
None yet
Development

No branches or pull requests

3 participants