feat: expose fetch option on ManagementClient#1352
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1352 +/- ##
==========================================
- Coverage 89.76% 89.75% -0.01%
==========================================
Files 431 431
Lines 20108 20107 -1
Branches 9782 9782
==========================================
- Hits 18049 18048 -1
Misses 2059 2059
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hi @tsushanth,
Thanks for tracking this down and putting together a clean fix 🙌!
The root cause analysis in the PR description is spot on, and the source change in ManagementClient.ts is exactly right.
A few things to fix before we merge:
Chores on the GitHub side
- Unverified commits: The repo requires signed commits. Please rebase with GPG/SSH-signed commits (
git rebase --exec 'git commit --amend --no-edit -S' HEAD~N) before we proceed. - Branch is out of date: please rebase on
master(ideally as part of the signed-commit rebase above, so it's one step).
Code feedback
-
Test file location:
The new test lands in tests/management/, but wrapper-level tests live insrc/management/tests/unit/(seemanagement-client-custom-domain.test.ts). These two suites have different environments;tests/runs ESM against the compiled package,src/management/tests/unit/runs CJS directly against source. The wrapper test belongs in the unit suite. -
JSDoc on fetch:
fetchinherits a doc comment from BaseClientOptions inBaseClient.ts, but that's Fern-generated and thinner than the proxy/retry/instrumentation/Workers language from the issue; worth adding an explicitJSDocoverride onManagementClientOptions.
The core fix is solid, just these before it's ready to merge 🙂.
Closes auth0#1330. The internal fetcher layer added `BaseClientOptions.fetch?: typeof fetch` in auth0#1238 and routes it through `fetcherImpl(args.fetchFn ?? …)`. The public wrapper, however, both omitted `fetch` from `ManagementClientOptions` and explicitly deleted it from the incoming options bag, so library consumers had no supported way to provide their own HTTP transport — even though the SDK is already capable of using one. - Drop `"fetch"` from the `Omit<FernClient.Options, …>` list so the field is part of the public surface. - Stop deleting `_options.fetch` in the `ManagementClient` constructor. The internal `fetcher` is still stripped (it's a Fern implementation detail), with a comment explaining why. Adds a focused test in `tests/management/ManagementClient.test.ts` that constructs a `ManagementClient` with a custom `fetch`, fires a request, and asserts the custom implementation was invoked against the expected URL. Also covers the no-`fetch` happy path to make sure the default global-fetch route is unchanged.
Follow-ups on top of the custom-fetch fix: - Move the wrapper test from tests/management/ into the unit suite (src/management/tests/unit/) alongside the other wrapper tests, and assert on the real rules.list() return value. - Add an explicit enriched JSDoc on the public `fetch` field. - Fix the adjacent class-level example to use `fetch:` instead of the `fetcher:` callback the constructor deletes.
5e96b0b to
ad61319
Compare
|
Hi @tsushanth! Thanks again for the original fix and root-cause work here. Since the review comments were still open and the branch had fallen behind the What I added on top of your commit:
Your core fix is unchanged and remains on your commit. Your contribution is much appreciated 🙌 |
|
After a discussion with @frederikprijck on approach: for consistency with the other Auth0 SDKs, we're going to expose a wrapped fetcher ( Since that's a different design and a larger change, it'll land as a separate PR rather than reworking this branch. Closing this one in favor of that - bu thanks @tsushanth for the root-cause work on #1330 and the original fix; it's what surfaced the whole issue and pointed at exactly the right layer. Credit carried over to the new PR. |
Why
Closes #1330.
The internal fetcher layer added `BaseClientOptions.fetch?: typeof fetch` in #1238 and routes it through `fetcherImpl(args.fetchFn ?? …)`. The public wrapper, however, both omitted `fetch` from `ManagementClientOptions` and explicitly deleted it from the incoming options bag, so library consumers had no supported way to provide their own HTTP transport — even though the SDK is already capable of using one. Typical use cases the reporter and downstream Auth0 users have called out: proxies / corporate egress, retry middlewares, OpenTelemetry instrumentation, and Cloudflare Workers / Bun targets where the global `fetch` needs swapping.
What
Tests
src/management/tests/unit/management-client-fetch.test.ts- two cases:Compat
Source-compatible: existing call sites that didn't pass `fetch` see no behavioural difference. Callers can now opt in:
```ts
new ManagementClient({
domain: 'tenant.auth0.com',
token: '...',
fetch: myInstrumentedFetch,
});
```