Open soartec-lab opened 1 month ago
Hi, @melloware What do you think this?
Sounds reasonable to me
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({...})
.
@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.
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'};
});
@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)
},
}
)
@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.
@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: ... });
});
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.
What were you expecting to happen?
I want to use it like below:
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.