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

v4.7.0 broke code generation #419

Closed
cvereterra opened this issue May 8, 2023 · 2 comments
Closed

v4.7.0 broke code generation #419

cvereterra opened this issue May 8, 2023 · 2 comments
Labels

Comments

@cvereterra
Copy link
Contributor

cvereterra commented May 8, 2023

Hi guys,

Mea culpa, my PR broke code generation, the tests didn't catch this.

@Xiphe I suggest rolling back to v4.6.0. In the meantime, I will try to fix this and add a PR with a test.

Given this endpoint definition

"paths": {
    "/healthcheck": {
      "get": {
        "responses": {
          "200": {
            "description": "OK",
            "content": {
              "application/json": {
                "schema": {
                  "properties": {
                    "message": {
                      "type": "string",
                      "example": "I'm health"
                    }
                  },
                  "required": ["message"]
                }
              }
            }
          }
        }
      }
    },
 ...
}

The response's schema should be:

{
    message: string;
}

Before v4.7.0, oazapfts generated

export function getHealthcheck(opts?: Oazapfts.RequestOpts) {
    return oazapfts.fetchJson<{
        status: 200;
        data: {
            message: string;
        };
    }>("/healthcheck", {
        ...opts
    });
}

Whereas now, it doesn't type the response

export function getHealthcheck(opts?: Oazapfts.RequestOpts) {
    return oazapfts.fetchJson<{
        status: 200;
        data: {};
    }>("/healthcheck", {
        ...opts
    });
}
@cvereterra
Copy link
Contributor Author

cvereterra commented May 8, 2023

Narrowed it down to the filtering done here in generate.ts (line 741). onlyMode is passed as readOnly since it's the result of a GET. The response schema is not flagged as readOnly by isSchemaReadOnly, so it get's filtered out.

 /**
   * Recursively creates a type literal with the given props.
   */
  getTypeFromProperties(
    props: {
      [prop: string]: SchemaObject | OpenAPIV3.ReferenceObject;
    },
    required?: string[],
    additionalProperties?:
      | boolean
      | OpenAPIV3.SchemaObject
      | OpenAPIV3.ReferenceObject,
    onlyMode?: OnlyMode
  ): ts.TypeLiteralNode {
    const members: ts.TypeElement[] = Object.keys(props)
      .filter((name) => {
        const schema = props[name];
        if (!onlyMode)
          return (
            !this.isSchemaReadOnly(schema) && !this.isSchemaWriteOnly(schema)
          );
        if (onlyMode === "readOnly") return this.isSchemaReadOnly(schema);
        else if (onlyMode === "writeOnly")
          return this.isSchemaWriteOnly(schema);
      })
      .map((name) => {
        const schema = props[name];
        const isRequired = required && required.includes(name);
        let type = this.getTypeFromSchema(schema, name);
        if (!isRequired && this.opts.unionUndefined) {
          type = factory.createUnionTypeNode([type, cg.keywordType.undefined]);
        }
        return cg.createPropertySignature({
          questionToken: !isRequired,
          name,
          type,
        });
      });

@cvereterra cvereterra mentioned this issue May 8, 2023
@Xiphe Xiphe closed this as completed in 44848a2 May 9, 2023
@github-actions
Copy link

github-actions bot commented May 9, 2023

🎉 This issue has been resolved in version 4.7.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Xiphe added a commit that referenced this issue Aug 14, 2023
the behaviour has been introduced to address #419
but it seems it's not required with the new implementation

fix #453
Xiphe added a commit that referenced this issue Aug 28, 2023
the behaviour has been introduced to address #419
but it seems it's not required with the new implementation

fix #453
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant