-
-
Notifications
You must be signed in to change notification settings - Fork 268
refactor(client-ky): flatten config and use top-level imports #2961
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
base: main
Are you sure you want to change the base?
refactor(client-ky): flatten config and use top-level imports #2961
Conversation
- Remove kyOptions namespace in favor of flat config extending ky.Options - Remove custom RetryOptions wrapper to expose full ky retry API (10 properties) - Change ky instance type from typeof ky to KyInstance for proper custom instance support - Use top-level imports instead of inline import() for better readability - Remove manual retry property mapping that limited functionality - Align with axios/fetch client patterns for consistency This gives users full access to ky's API including advanced retry features like jitter, backoffLimit, custom delay functions, and shouldRetry callbacks.
|
|
|
|
@SebastiaanWouters is attempting to deploy a commit to the Hey API Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2961 +/- ##
==========================================
- Coverage 29.41% 29.38% -0.04%
==========================================
Files 403 403
Lines 35766 35748 -18
Branches 2066 2063 -3
==========================================
- Hits 10522 10504 -18
Misses 25217 25217
Partials 27 27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
commit: |
| >, | ||
| CoreConfig { | ||
| /** | ||
| * Base URL for all requests made by this client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SebastiaanWouters Why remove these comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a fan of many verbose comments, code should explain itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a user-facing interface, comments are a must, you don't want people to dig into code unless something goes wrong!
| signal: opts.signal, | ||
| throwHttpErrors: opts.throwOnError ?? false, | ||
| timeout: opts.timeout, | ||
| ...opts.kyOptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SebastiaanWouters Will this still work since opts isn't being spread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes all options are being plucked from the opts. opts.kyOptions is no longer a thing. However, the spreading can be moved to opts (for most props)
Summary
Addresses feedback from #2958 to simplify the ky client configuration by removing the namespace approach.
Changes
kyOptionsnamespace - Config now directly extendsky.OptionsRetryOptionswrapper - Users now have access to all 10 ky retry properties instead of just 3import('ky')to properimport type { KyInstance, Options } from 'ky'ky?: typeof kytoky?: KyInstancefor proper custom instance supportBenefits
ky.create()Before
After
Test Plan