feat: add --argumentStyle option

[Liooo]

abstract

This PR adds an option that lets us opt in to object style argument by --argumentStyle=object:

export function updatePetWithForm({ petId, body }: {
    petId: number;
    body?: {
        /** Updated name of the pet */
        name?: string;
        /** Updated status of the pet */
        status?: string;
    };
}, opts?: Oazapfts.RequestOpts) {
    return oazapfts.fetchText(`/pet/${encodeURIComponent(petId)}`, oazapfts.form({
        ...opts,
        method: "POST",
        body
    }));
}

as opposed to --argumentStyle=positional one (which is default):

export function updatePetWithForm(petId: number, body?: {
    /** Updated name of the pet */
    name?: string;
    /** Updated status of the pet */
    status?: string;
}, opts?: Oazapfts.RequestOpts) {
    return oazapfts.fetchText(`/pet/${encodeURIComponent(petId)}`, oazapfts.form({
        ...opts,
        method: "POST",
        body
    }));
}

motivation

• most of other libraries support this by default or as opt-in, and I guess not few ppl like this style
• sometimes it's kinder to typing (e.g. we can now derive the type of whole request param by Parameters<typeof func>[0], instead of struggling with a tuple of variable length)

a few other points

• while the word positional argument is somewhat canonical, couldn't find good one for object style (technically speaking this is just destructuring assignment in function argument)
• didn't take care of property name duplication between requestBody and parameters, I'll add the logic if this is crucial
• additional tests are pretty minimal, tell me if there's any important cases
• in my initial implementation, the object argument types were defined as separate type aliases and export-ed, if this is wanted I'm happy to revert this behavior but as another option, maybe --argumentStyle=objectExported, or add new one like --exportObjcectArgumentTypes=true

P.S. Thanks for the great work. As far as I know, oazapfts is the best option for openapi ts codegen out there, in terms of generated code tidiness, supported openapi features (kudos to readOnly/writeOnly support) and all. I think this library should get more attention.

[Xiphe]

Hi @Liooo! Thank you for the contribution! This is looking great already!

Can you add a --argumentStyle=object variant to the npm run generate-demo script and check in the artifact?

I will try to find time for a deeper review in the coming days 👍

[Liooo]

@Xiphe

Thanks, I ran generate-demo. I found an issue that when there's no input for the endpoint an empty object argument is generated, so fixed that and added some tests. Apart from that it's looking good to me.

Please check out when you've got time :)

[Xiphe]

Great Job with this. I think the implementation in generate.ts can be organized a little cleaner other then that LGTM

[Xiphe]

Since the generateApi method already is quite convoluted. Can we refactor the body of this switch to standalone methods/functions such as getPositionalMethodParams and getObjectMethodParams?

[Xiphe]

This seems to be duplicated in both switch cases, can we keep remove the duplication?

[Liooo]

Hey thanks for the review 👋 I've looked into them but,

Can we refactor the body of this switch to standalone methods/functions

the switch block depends on getArgName() which ultimately depends on a loop variable item, so we need to either handle all these dependencies as well, or define inline function to closure-access these guys.

this seems to be duplicated in both switch cases, can we keep remove the duplication?

the last .push part slightly differs, so if we're extracting the common part naively, we'll have something like:

// the method name is temporary
const getBodyish = (requestBody: OpenAPIV3.ReferenceObject | OpenAPIV3.RequestBodyObject) => {
  //..brah
  return { body, bodyVar, type };
};

then since body and bodyVar are referenced later on, the caller side will be:

let body, bodyVar;

switch(argumentStyle) {
  case 'positional':
    // ..brah

    if (requestBody) {
      let type: TypeNode;
      ({body, bodyVar, type} = getBodyish(requestBody))

      // ..
    }

    // ..brah
    
  case 'object':
    // ..brah

    if (requestBody) {
      let type: TypeNode;
      ({body, bodyVar, type} = getBodyish(requestBody))

      // ..
    }

    // ..brah
}

or the maybe the if (requestBody) { ({body, bodyVar, type} = getBodyish() } equivalent part can be moved above switch block, but then inside the switch we'll need to make sure the all of getBodyish return values are not falsey to prevent type error.

I'm happy to keep going, but IMHO it better either stay as is for now, or expand the refactoring scope and apply bit of structural change inside generateApi() later, rather than pre-optimizing this specific part right now. (After all the method is long but cohesive, wasn't hard to read.)

What do you think?

[Xiphe]

Hi @Liooo

Thank you for the explanation haven't seen those details you're mentioning. Gonna merge this now and open a refactor ticket.

[github-actions]

🎉 This PR is included in version 5.1.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀