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

Adds option to disable client auto creation in SDK #1526

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chriswiggins
Copy link
Contributor

This PR adds an option to the SDK which allows the user to disable the auto-creation of the client in the SDK output. (i.e not doing a const client = createClient(createConfig()); at the top)

Reasoning behind this is we are generating an API client for a microservices based endpoint, so there are multiple SDKs that need to be generated. We want to sit these behind another class where the client is instantiated once, rather than multiple times.

Based on this, when you set autoCreateClient: false, the following happens:

  • If asClass is true:
    • The SDK Class output now requires instantiation
    • The constructor must be passed a client argument
    • Each method is no longer static, as the client from the class is used for each call
  • If flat:
    • Each method now has a required options argument
    • The client property is now also required in the options

I'm not sure I like the autoCreateClient variable name, so input would be appreciated!

Copy link

stackblitz bot commented Jan 1, 2025

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

changeset-bot bot commented Jan 1, 2025

🦋 Changeset detected

Latest commit: a0308ab

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hey-api/openapi-ts Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Jan 1, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hey-api-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 1, 2025 6:38am

Copy link

codecov bot commented Jan 1, 2025

Codecov Report

Attention: Patch coverage is 39.35484% with 94 lines in your changes missing coverage. Please review.

Project coverage is 63.18%. Comparing base (8d563b5) to head (a0308ab).

Files with missing lines Patch % Lines
...ages/openapi-ts/src/plugins/@hey-api/sdk/plugin.ts 46.90% 60 Missing ⚠️
packages/openapi-ts/src/compiler/types.ts 5.55% 34 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1526      +/-   ##
==========================================
- Coverage   63.35%   63.18%   -0.18%     
==========================================
  Files         150      150              
  Lines       22748    22860     +112     
  Branches     1864     1869       +5     
==========================================
+ Hits        14412    14443      +31     
- Misses       8329     8411      +82     
+ Partials        7        6       -1     
Flag Coverage Δ
unittests 63.18% <39.35%> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

pkg-pr-new bot commented Jan 1, 2025

Open in Stackblitz

npm i https://pkg.pr.new/@hey-api/client-axios@1526
npm i https://pkg.pr.new/@hey-api/client-fetch@1526
npm i https://pkg.pr.new/@hey-api/openapi-ts@1526

commit: a0308ab

@mrlubos
Copy link
Member

mrlubos commented Jan 1, 2025

Nice to see you again 😀 I've got a few questions first:

Which approach are you using on your project? Class or flat?

Why not make the class methods behave the same way as flat functions, i.e. require client on every call?

@chriswiggins
Copy link
Contributor Author

chriswiggins commented Jan 1, 2025

Which approach are you using on your project? Class or flat?

Class 🙂 (EDIT: Sorry I had written flat before by accident)

Why not make the class methods behave the same way as flat functions, i.e. require client on every call?

Because then I'd have to pass the client in every time, rather than just calling the methods without any options. Slightly more efficient I guess (and I prefer programming that way)

@mrlubos
Copy link
Member

mrlubos commented Jan 4, 2025

@chriswiggins I'll need to think about this, and it's not been a priority so far. This goes into the "full SDK" territory which has been discussed many times (e.g. #940). The main issue in this pull request is that it introduces an inconsistent API for flat/class users and I'd rather explore the solution further before possibly committing to this approach. You're totally free to use the fork or even this pull request release in the meantime as I assume you want to use this ASAP. Sorry for the delay here!

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.

2 participants