Skip to content

feat: add injectable fetch, abort propagation, and selective SDK publishing - #30

Open
akshitsinha wants to merge 6 commits into
mainfrom
asinha/injectable-fetch-abort-signal
Open

feat: add injectable fetch, abort propagation, and selective SDK publishing#30
akshitsinha wants to merge 6 commits into
mainfrom
asinha/injectable-fetch-abort-signal

Conversation

@akshitsinha

@akshitsinha akshitsinha commented Aug 6, 2026

Copy link
Copy Markdown
Member

Summary

Improves the TypeScript SDK's HTTP evaluation and makes releases SDK-aware.

TypeScript SDK

  • Adds injectable fetch transports at client and per-call levels.
  • Propagates caller and fetchOptions abort signals to the HTTP request.
  • Distinguishes caller aborts from timeouts with FlagshipErrorCode.ABORTED and exposes whether errors are retryable.
  • Cancels retry delays immediately on abort, releases discarded response bodies, and optimizes evaluation URL construction.

Release automation

  • Publishes npm only for publish-relevant TypeScript changes.
  • Publishes PyPI only for publish-relevant Python changes.
  • Creates Go module tags only for publish-relevant Go changes.
  • Includes source, package configuration, examples, documentation, and license changes; excludes mechanical release PR version bumps and test- or lockfile-only changes.
  • Uses idempotent SDK-specific release tags so failed native publishes remain retryable.

Existing callers that provide no custom transport or signal remain compatible.

…tion

Add `FlagshipProviderOptions.fetch` and per-call `evaluate(..., { fetch, signal })`
so consumers can route evaluations through a Workers service binding or stub the
transport in tests without mutating `globalThis.fetch`.

Caller signals are now merged with the request timeout and with
`fetchOptions.signal` (previously discarded), so an abort cancels the in-flight
HTTP request. Caller aborts surface as the new `FlagshipErrorCode.ABORTED` and
are never retried; timeout aborts keep their existing retry behaviour.

`FlagshipError.retryable` exposes whether a failure was transient. 408, 425, 429
and 5xx are retryable; other non-2xx responses are now terminal instead of
retried.
@akshitsinha
akshitsinha requested review from a team, palashgo and roerohan as code owners August 6, 2026 10:08
@changeset-bot

changeset-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: e91a136

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

This PR includes changesets to release 1 package
Name Type
@cloudflare/flagship 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

@pkg-pr-new

pkg-pr-new Bot commented Aug 6, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@cloudflare/flagship@30

commit: e91a136

@akshitsinha akshitsinha changed the title feat(typescript): injectable fetch transport and caller abort propagation feat: add injectable fetch, abort propagation, and selective SDK publishing Aug 7, 2026
@akshitsinha

Copy link
Copy Markdown
Member Author

/bonk review this PR

@cloudflare cloudflare deleted a comment from ask-bonk Bot Aug 7, 2026
@cloudflare cloudflare deleted a comment from ask-bonk Bot Aug 7, 2026
@cloudflare cloudflare deleted a comment from ask-bonk Bot Aug 7, 2026
@ask-bonk

ask-bonk Bot commented Aug 7, 2026

Copy link
Copy Markdown

I've now thoroughly reviewed all 25 changed files, the full diff, and validated the build. Here's my review:


Review Summary

This is a well-structured PR that adds two related TypeScript SDK features (injectable fetch transport, caller AbortSignal propagation) and overhauls the release automation to publish only changed SDKs. All checks pass: formatting, linting, typecheck, and 304 tests + 11 release-automation tests.

TypeScript SDK changes

Strengths:

  • The signal-merging design is solid. mergeSignals prefers AbortSignal.any where available and falls back to a manually linked controller, with proper dispose() cleanup. The fallback path is tested explicitly.
  • The retryable property on FlagshipError is a good API addition. Moving retry classification from status-code checks scattered in fetchWithRetry to a single isRetryableStatus + a boolean on the error object is cleaner and more composable for consumers.
  • discardResponse and waitForRetry both handle abort-during-delay correctly, and the test at client.test.ts:457 ("aborts while waiting to retry") validates the edge case well.
  • The buildEvaluationUrl fast path (client.ts:44 / context.ts:44) that avoids new URL() when the base has no ? or # is a reasonable optimisation for the hot path, and the old ContextTransformer.buildUrl delegates to it so backward compatibility is preserved.
  • The FlagshipError default retryable: false is the safe choice — only explicitly retryable paths opt in.

Findings:

  1. fetchOptions.signal is spread into the request but also merged separately (client.ts:37 stores it, client.ts:68 adds it to the signals array, and client.ts:128 spreads this.options.fetchOptions which still contains the original signal). The merged signal replaces it in the spread (signal: merged.signal wins over ...this.options.fetchOptions), so this works correctly because object spread order puts signal after the spread. This is intentional but subtle — a brief comment at the spread site noting that signal intentionally overrides fetchOptions.signal would aid future readers. Not a bug.

  2. ContextTransformer.toQueryParams now returns from URLSearchParams entries (context.ts:25). Object.fromEntries(toSearchParams(...)) silently deduplicates keys (last value wins) while the old loop did too (plain object assignment). Behaviour is identical, but worth noting that this is a deliberate trade-off.

  3. resolveEndpoint now returns new URL(endpoint).toString() (client.ts:286) which canonicalises the URL. For example, https://api.example.com becomes https://api.example.com/ (trailing slash added). This is a minor behavioural change — the buildEvaluationUrl fast path will append ?params to the canonicalised form. The context test at context.test.ts:201 validates this (https://api.example.comhttps://api.example.com/?flagKey=...). This is correct URL semantics but could surprise callers who previously saw their literal endpoint echoed back.

  4. No FlagshipClientProvider forwarding of fetch/signal: The web provider re-exports FlagshipRequestOptions but the changeset notes "FlagshipClientProvider accept and forward fetch in HTTP mode". I don't see client-provider.ts in the diff — it presumably already constructs a FlagshipClient from its options, and since FlagshipProviderOptions now includes fetch, this should flow through. Worth confirming the client provider's constructor passes it along.

Release automation changes

Strengths:

  • classifySdkChanges is well-tested: test-only changes, lockfiles, examples, licenses, and build config are all classified correctly. The "ignores mechanical version bumps" test is particularly important for avoiding release loops.
  • The detectSdkChanges function using per-SDK tags as baselines (falling back to the canonical tag) correctly handles the case where a native publish fails and needs to be retried on the next release.
  • Separating changeset publish (npm) from changeset tag (canonical tag) and only running publish when TypeScript changed is clean.

Finding:

  1. publish-pypi.yml pins a different setup-uv version than release.yml. The PyPI workflow uses setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b (v8.1.0 per the comment) while release.yml uses setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 (v8.2.0). This is not introduced by this PR if the PyPI file wasn't changed, but since both files are in the diff, it's worth aligning them to the same version.

Minor / style observations

  • The pnpm-lock.yaml changes are large (+768/-811) but consistent with the dev dependency version bumps in package.json.
  • Test formatting changes in binding-provider.test.ts and server-provider-cache.test.ts (removing unnecessary wrapping parentheses on arrow functions) are pure oxfmt reformats — no logic changes.
  • The .changeset/README.md wording update accurately reflects the new selective publishing behaviour.
  • AGENTS.md updates are minimal and match the new release flow.

Verdict

The code is correct, well-tested, and backward-compatible. No blocking issues found. The five findings above are informational or minor improvement suggestions, none requiring changes before merge.

github run

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.

1 participant