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

Upgrade openapi-typescript to 7.0.2 + swap to openapi-fetch #18532

Merged
merged 149 commits into from
Aug 15, 2024

Conversation

davelopez
Copy link
Contributor

@davelopez davelopez commented Jul 11, 2024

Upgrade to the latest version of openapi-typescript. This introduces some breaking changes in particular:

Most optional objects are now always present in types, just typed as :never. This includes keys of the Components Object as well as HTTP methods.

Maybe this is a good opportunity to continue with @dannon's work started in https://github.com/dannon/galaxy/commits/openapi-typescript-fetch-upgrade/ and swap to openapi-fetch

Update

See discussion details at #18534

TLDR; There are some more incompatibilities between the latest openapi-typescript generated schemas and the current openapi-typescript-fetcher so, if we want to upgrade we should replace the current fetcher with the most up-to-date openapi-fetch variant.

TODO


New way of querying the API

For more information see the updated client docs.

import { ref, onMounted } from "vue";

import { GalaxyApi, type HDADetailed } from "@/api";
import { errorMessageAsString } from "@/utils/simple-error";

interface Props {
    datasetId: string;
}

const props = defineProps<Props>();

const datasetDetails = ref<HDADetailed>();
const errorMessage = ref<string>();

async function loadDatasetDetails() {
    // Your IDE will provide you with autocompletion for the route and all the parameters
    const { data, error } = await GalaxyApi().GET("/api/datasets/{dataset_id}", {
        params: {
            path: {
                dataset_id: props.datasetId,
            },
            query: { view: "detailed" },
        },
    });

    if (error) {
        // Handle error here. For example, you can display a message to the user.
        errorMessage.value = errorMessageAsString(error);
        // Make sure to return here, otherwise `data` will be undefined
        return;
    }

    // Use `data` here. We are casting it to HDADetailed to help the type inference because
    // we requested the "detailed" view and this endpoint returns different types depending
    // on the view. In general, the correct type will be inferred automatically using the
    // API schema so you don't need to (and shouldn't) cast it.
    datasetDetails.value = data as HDADetailed;
}

onMounted(() => {
    loadDatasetDetails();
});

New way of mocking the API responses in the client unit test

Along with the openapi-fetch replacement, we introduced a new way of mocking API calls in the client using Mock Service Worker.

For more information see the updated docs.

import { useServerMock } from "@/api/client/__mocks__";

const { server, http } = useServerMock();

describe("MyComponent", () => {
    it("should do something with the history", async () => {
        // Mock the response of the API call
        server.use(
            http.get("/api/histories/{history_id}", ({ params, query, response }) => {
                // You can use logic to return different responses based on the request
                if (query.get("view") === "detailed") {
                    return response(200).json(TEST_HISTORY_DETAILED);
                }

                // Or simulate an error
                if (params.history_id === "must-fail") {
                    return response("5XX").json(EXPECTED_500_ERROR, { status: 500 });
                }

                return response(200).json(TEST_HISTORY_SUMMARY);
            })
        );

        // Your test code here
    });
});

How to test the changes?

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@davelopez davelopez added kind/enhancement kind/refactoring cleanup or refactoring of existing code, no functional changes area/client labels Jul 11, 2024
@mvdbeek
Copy link
Member

mvdbeek commented Jul 11, 2024

I think this looks pretty good, what's a clear improvement is that default values and constants don't automatically make attributes optional:

              * Ext
              * @default auto
              */
-            ext?: string;
+            ext: string;

@davelopez
Copy link
Contributor Author

Should I give it a try to swap to openapi-fetch directly here then? Or try to fix all the "never" issues in the current openapi-typescript-fetch? 🤔

@mvdbeek
Copy link
Member

mvdbeek commented Jul 11, 2024

I don't know what's better, I assume all these things need to be fixed whether we use openapi-fetch or not ?

@davelopez
Copy link
Contributor Author

I assume all these things need to be fixed whether we use openapi-fetch or not ?

Yes, but if we plan to move to openapi-fetch in the future, we will have to replace most if not all fetcher calls anyway so it would be fixing something that we are going to replace later.

I think I will give it a try to replace it, and if it is too disruptive I will fall back and just fix the typing errors.

@dannon
Copy link
Member

dannon commented Jul 11, 2024

@davelopez please do just go ahead with that and swap over; I think it's the right move and I'd be more than happy for you to take over!

@davelopez davelopez changed the title Upgrade openapi-typescript to 7.0.2 Upgrade openapi-typescript to 7.0.2 + swap to openapi-fetch Jul 12, 2024
@davelopez davelopez force-pushed the upgrade_openapi_typescript branch 3 times, most recently from 71e2f18 to f7fdac6 Compare July 16, 2024 09:17
@davelopez davelopez force-pushed the upgrade_openapi_typescript branch 2 times, most recently from 2720d7b to cb67b8e Compare July 18, 2024 17:02
@davelopez davelopez force-pushed the upgrade_openapi_typescript branch from cb67b8e to 2f84034 Compare July 23, 2024 13:42
@davelopez davelopez force-pushed the upgrade_openapi_typescript branch from 2f84034 to 267ba03 Compare July 25, 2024 16:26
@davelopez davelopez force-pushed the upgrade_openapi_typescript branch from 267ba03 to 7d0a276 Compare July 26, 2024 18:13
@davelopez davelopez force-pushed the upgrade_openapi_typescript branch 4 times, most recently from 01df758 to 296408a Compare August 4, 2024 10:27
@davelopez davelopez force-pushed the upgrade_openapi_typescript branch from 296408a to 81bdce2 Compare August 6, 2024 13:21
davelopez and others added 21 commits August 13, 2024 13:23
As a "simple" example on how to handle the API error.
And avoid leaving the async call unresolved
With server mock and missing API configuration endpoint. The missing API mock may cause the test to not handle the rejected promise otherwise.
With server mock and missing API configuration endpoint. The missing API mock may cause the test to not handle the rejected promise otherwise.
Minimum error handling, it probably needs better error handling logic and display some feedback to the user.
This will avoid import side effects and will enable easy API mocking in unit tests in the future.
To use errorMessageAsString consistently
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do this. Thanks a lot for working through all of the details, this is great!

@mvdbeek
Copy link
Member

mvdbeek commented Aug 14, 2024

I'll leave this open till tomorrow to give others a chance to review

Copy link
Member

@dannon dannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me, thanks so much @davelopez for picking this up and running with it.

@mvdbeek mvdbeek merged commit 2dcd285 into galaxyproject:dev Aug 15, 2024
52 of 54 checks passed
@davelopez davelopez deleted the upgrade_openapi_typescript branch August 15, 2024 14:30
@ahmedhamidawan ahmedhamidawan added the highlight/dev Included in admin/dev release notes label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client highlight/dev Included in admin/dev release notes kind/enhancement kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants