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

fix(mock/msw): override response if it's false as well #1313

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

almyy
Copy link
Contributor

@almyy almyy commented Apr 16, 2024

Status

READY

Description

If you pass the value false to a mock handler to override response with that value, it would fallback to the default response. This PR should fix this, and it will now accept any value that isn't undefined.

Todos

  • Tests
  • Documentation (N/A)
  • Changelog Entry (unreleased)

@melloware melloware requested a review from soartec-lab April 16, 2024 11:26
@melloware melloware added the mock Related to mock generation label Apr 16, 2024
@melloware melloware added this to the 6.28.0 milestone Apr 16, 2024
@melloware melloware merged commit b13df01 into orval-labs:master Apr 16, 2024
2 checks passed
@soartec-lab
Copy link
Member

soartec-lab commented Apr 16, 2024

@almyy
Originally, it was assumed that an object would be passed as an argument. The object to be mocked is a JSON API, and there was no expectation that a single false would be returned.
Therefore, the practicality benefits of this change may be outweighed by the increased complexity of automatically generated source code.

Was there a specific use case?

@almyy
Copy link
Contributor Author

almyy commented Apr 16, 2024

@soartec-lab
We have an endpoint in our API that returns a boolean value, which in itself might be bad API design (but completely valid json), but the point is that the mock-handler that gets generated from this openAPI definition:

"/api/someEndpoint": {
    "get": {
        "tags": ["some-tag"],
        "operationId": "someEndpoint",
        "responses": {
            "200": {
                "description": "OK",
                "content": {"application/json;charset=UTF-8": {"schema": {"type": "boolean"}}}
            }
        }
    }
},

looks like this:

export const getSomeEndpointMockHandler = (overrideResponse?: boolean) => {
    return http.get("*/api/someEndpoint", async () => {
        await delay(1000);
        return new HttpResponse(
            JSON.stringify(overrideResponse ? overrideResponse : getSomeEndpointResponseMock()),
            {
                status: 200,
                headers: {
                    "Content-Type": "application/json",
                },
            }
        );
    });
};

As you can see, if I use this handler: getSomeEndpointMockHandler(false), it would not actually use the overridden response, because it's false.

If you think this change isn't practical enough, I can always create my own handler, which I in fact already am doing because of this "bug" 😄 That being said, I think checking specifically for a non-existent value (undefined) is cleaner, as there might be use-cases for passing other falsy values here as well (empty string?, 0?).

@melloware
Copy link
Collaborator

I agree with @almyy as falsiness prevents a valid value of "false".

@soartec-lab
Copy link
Member

@almyy @melloware

thank you for let me know. I understand that.

An API that returns only false is correct as a specification, and it actually exists in the API you are developing.
I had never come across an example of this so I had doubts but, if it complies with the specifications for OpenAPI, I think it should be respected. so I think it's reasonable for it to be fixed.

Thanks again for the correction 🙌

@soartec-lab soartec-lab added the bug Something isn't working label Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mock Related to mock generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants