Skip to content

refactor: give the REST layer one transport seam instead of four verb methods - #262

Closed
diegolmello wants to merge 2 commits into
mobilefrom
arch/03
Closed

refactor: give the REST layer one transport seam instead of four verb methods#262
diegolmello wants to merge 2 commits into
mobilefrom
arch/03

Conversation

@diegolmello

@diegolmello diegolmello commented Aug 12, 2026

Copy link
Copy Markdown
Member

Proposed changes

The REST layer named the HTTP verb set twice. Api.request took a method string and expanded it through a four-branch switch into four differently-named methods on IClient; each of those collapsed the verb straight back down into the method: field of a fetch call — after duplicating URL assembly, header selection, body encoding, signal plumbing and response parsing a fourth time. IClient was a shallow interface: four members whose only difference was one string.

This replaces it with a single seam. Api now builds a REST request value — method, endpoint, data, options, API version — and hands it to transport.send(request). The switch is gone, and URL, headers, body, signal and parsing exist exactly once. Adding a verb, a retry, or a tracing header is now a one-place change instead of a four-place one, and a fake transport in a test is one function instead of four.

Two judgement calls worth recording:

  • Named REST transport, not Client. CONTEXT.md already binds Client to the object a consuming app holds to reach a server, covering both REST and realtime. The thing underneath Api is the peer of Driver, not of Client, so keeping the old name would have mis-labelled the new seam on day one. REST request and REST transport are added to CONTEXT.md — the REST half of the SDK now has the vocabulary the realtime half already had. The rename is safe here: IClient and Api.client were referenced nowhere outside the file they were declared in, and neither is re-exported from the package entry point. The one visible consequence is that the Api constructor now takes transport where it took client; nothing in this repo passes either.
  • The Api constructor option is renamed, and that is a breaking change. It now takes transport where it took client, and the public field Api.client is now Api.transport. RocketChatClient forwards ...config down to super, so a consumer still passing client gets no error — the option is simply not recognised and a default transport is built instead. Nothing in this repo passes either, and neither IClient nor the field was re-exported from the package entry point, but a consuming app that injects its own client has to rename the option.
  • The old default: fallthrough to POST is gone. The previous switch funnelled any unrecognised method into a POST; the method now goes straight to fetch. TypeScript already constrained this to the four verbs, and silently turning a typo into a POST hid mistakes rather than preventing them. A test pins that each verb passes through untouched.
  • The lifetime of AbortController was left alone. Api builds one in its constructor and never replaces it, so once abort() is called every later request is born cancelled. That is a real defect, but it is a behaviour question rather than a structural one, and a refactor should be provably behaviour-preserving. It wants its own change.

Also removes ~60 lines of commented-out login/logout/setAuth/clearHeaders at the top of the file, which a live login/logout further down had already replaced. Same file, same layer, and it forced any reader to work out which of the two implementations was real before reading anything else.

Behaviour is otherwise unchanged: the same URLs, headers, bodies and query encoding go on the wire, including the trailing ? a GET with an empty payload has always produced, and a GET given an explicitly null payload still throws where it always did.

Steps to reproduce

  • Log in through the SDK and issue a GET, POST, PUT and DELETE against a server; each reaches the same URL, with the same headers and body, as before the change.
  • Issue a GET with an array value in the payload; it is still encoded as repeated key[]= params.
  • Pass customHeaders in a request's options; it still replaces the default headers for that request alone rather than merging with them.
  • Upload FormData; it is still passed to fetch unencoded.
  • Call abort(); in-flight requests are still cancelled through the signal Api threads into every request.

Tests

  • lib/api/__tests__/api.spec.ts is new — the REST layer had no coverage at all. It pins both sides of the new seam:
  • puts a GET payload in the query string and sends no body, including repeated key[]= params for arrays
  • sends a JSON body for the write methods
  • passes FormData through unencoded
  • honours the requested API version
  • merges the settings custom headers into every request
  • lets a per-request customHeaders option replace the headers entirely
  • sends the auth headers a login established
  • defaults the host when none is given
  • describes the request as a value rather than choosing a transport method per verb
  • threads its abort signal into every request, and marks it aborted on abort()
  • sends an authenticated request even before login — pinning the existing quirk that loggedIn() answers yes for an absent login, since every() over no keys is vacuously true
  • rejects with the result when the status is a failure, and resolves when the caller passed an ignore pattern matching it
  • returns the whole result for a DELETE that carries no data
  • passes each verb through untouched, with no fallback to POST

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