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

Axios removed from js client #348

Closed
wants to merge 12 commits into from
Closed

Conversation

gustawdaniel
Copy link

As suggested in #254 I replaced axios by fetch

@jeffchuber
Copy link
Contributor

@gustawdaniel this is really cool! thanks for doing this.

We are currently using codegen for the base typescript types based on the OpenAPI spec from FastAPI.

https://github.com/chroma-core/chroma/blob/main/clients/js/package.json#L40

To fully land this, we would want to move this over from typescript-axios to https://openapi-generator.tech/docs/generators/typescript-fetch.

Would you be willing to take a look at that?

@gustawdaniel
Copy link
Author

gustawdaniel commented Apr 15, 2023

I fixed this problem OpenAPITools/openapi-generator#3869

now see

export function UpdateEmbeddingToJSON(value?: UpdateEmbedding | null): any {
    if (value === undefined) {
        return undefined;
    }
    if (value === null) {
        return null;
    }
    return {
        
        'embeddings': value.embeddings,
        'metadatas': Array<any> | objectToJSON(value.metadatas),
        'documents': string | Array<any>ToJSON(value.documents),
        'ids': string | Array<any>ToJSON(value.ids),
        'increment_index': value.incrementIndex,
    };
}

or

.map(string | numberToJSON)

but I hope I will fix it.

@gustawdaniel
Copy link
Author

gustawdaniel commented Apr 15, 2023

@jeffchuber

why

mkfifo openapi.json; (curl -s 'http://localhost:8000/openapi.json' > openapi.json &)

instead of simpler

curl -s 'http://localhost:8000/openapi.json' > openapi.json 

What is problem now:

We have schema openapi 3.0.2. We trying to generate client using openapi-generator-cli. Unfortunately from this part of schema "anyOf": [ { "type": "string" }, { "type": "integer" } ] we getting code map(string | numberToJSON) that is incorrect.

This problem was reported here OpenAPITools/openapi-generator#2845 (comment)

There is support table for typescript-fetch:

https://github.com/OpenAPITools/openapi-generator/blob/master/docs/generators/typescript-fetch.md

and we can see that anyOf seems to be not supported now

support

there is our openapi schema

          "loc": {
            "title": "Location",
            "type": "array",
            "items": {
              "anyOf": [
                {
                  "type": "string"
                },
                {
                  "type": "integer"
                }
              ]
            }
          },

and generated not correct code

        'loc': ((value.loc as Array<any>).map(string | numberToJSON)),

for axios I see this code generated

    /**
     * 
     * @type {Array<string | number>}
     * @memberof ValidationError
     */
    loc: Array<string | number>;

@gustawdaniel
Copy link
Author

Options

  1. Write hacky code in chroma that simply remove .map(string | numberToJSON) from generated files
  2. close this issue and revert to axios-typescript as more stable
  3. add correct support of this feature to openapi-generator typescript-fetch

@gustawdaniel
Copy link
Author

gustawdaniel commented Apr 15, 2023

I tried openapi-generator-plus but without success

karlvr/openapi-generator-plus#41

main problem:

  • only node-fetch or window.fetch
  • we need code when fetch is taken from main context without any external library and window context

@gustawdaniel
Copy link
Author

Finally I replaced

        "@openapitools/openapi-generator-cli": "^2.6.0",

by

        "openapi-generator-plus": "^2.6.0",

but i have to patch it now because your schema is incorrect.

In openapi.json there are

    "/api/v1/version": {
      "get": {
        "summary": "Version",
        "operationId": "version",
        "responses": {
          "200": {
            "description": "Successful Response",
            "content": {
              "application/json": {
                "schema": {}
              }
            }
          }
        }
      }
    },

that are not correct openapi schema.

The OpenAPI schema you provided appears to be incomplete. Specifically, the "schema" object within the "application/json" content object is empty, which means that the response body will not contain any data.

If the response body should be empty, the schema object should be omitted entirely. However, if the response should contain data, you will need to define the schema to describe the structure of the data that will be returned.

Here is an example of a valid OpenAPI schema for a response with a JSON body:

"/api/v1/version": {
  "get": {
    "summary": "Version",
    "operationId": "version",
    "responses": {
      "200": {
        "description": "Successful Response",
        "content": {
          "application/json": {
            "schema": {
              "type": "object",
              "properties": {
                "version": {
                  "type": "string",
                  "example": "1.0.0"
                }
              }
            }
          }
        }
      }
    }
  }
}

In this example, the schema object defines an object with a single property "version" which is a string with an example value of "1.0.0".

@gustawdaniel
Copy link
Author

@jeffchuber there are details of all fixes that have to made in openapi.json

https://www.diffchecker.com/OtKP9Ogd/

potentially there can be better models than null. I added null because without it openapi-generator-plus that is more restrictive not works.

@gustawdaniel
Copy link
Author

gustawdaniel commented Apr 15, 2023

@jeffchuber I finished but think that these operations

sed -i 's/"schema": {}/"schema": {"type": "object"}/g' openapi.json
sed -i 's/"items": {}/"items": { "type": "object" }/g' openapi.json
sed -i -e 's/"title": "Collection Name"/"title": "Collection Name","type": "string"/g' openapi.json

need to be considered as fixing not fully covered schema, and they should be removed in future when schema will be fully and correctly described.

On the other hand

sed -i -e '/import "whatwg-fetch";/d' -e 's/window.fetch/fetch/g' src/generated/runtime.ts

is relatively low cost of usage quite good typescript fetch generator

@openapi-generator-plus/typescript-fetch-client-generator

@jeffchuber
Copy link
Contributor

@gustawdaniel thanks for all of this! will pull down and take a look tonight

@jeffchuber
Copy link
Contributor

@gustawdaniel mostly reviewed this - thanks for doing all of this. will wrap up today

@jeffchuber
Copy link
Contributor

closing in favor of #409

@jeffchuber jeffchuber closed this May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants