refactor: give the REST layer one transport seam instead of four verb methods - #262
Closed
diegolmello wants to merge 2 commits into
Closed
refactor: give the REST layer one transport seam instead of four verb methods#262diegolmello wants to merge 2 commits into
diegolmello wants to merge 2 commits into
Conversation
…port, restore the empty-query path
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
The REST layer named the HTTP verb set twice.
Api.requesttook a method string and expanded it through a four-branchswitchinto four differently-named methods onIClient; each of those collapsed the verb straight back down into themethod:field of afetchcall — after duplicating URL assembly, header selection, body encoding, signal plumbing and response parsing a fourth time.IClientwas a shallow interface: four members whose only difference was one string.This replaces it with a single seam.
Apinow builds a REST request value — method, endpoint, data, options, API version — and hands it totransport.send(request). Theswitchis 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:
REST transport, notClient.CONTEXT.mdalready binds Client to the object a consuming app holds to reach a server, covering both REST and realtime. The thing underneathApiis 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 toCONTEXT.md— the REST half of the SDK now has the vocabulary the realtime half already had. The rename is safe here:IClientandApi.clientwere 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 theApiconstructor now takestransportwhere it tookclient; nothing in this repo passes either.Apiconstructor option is renamed, and that is a breaking change. It now takestransportwhere it tookclient, and the public fieldApi.clientis nowApi.transport.RocketChatClientforwards...configdown tosuper, so a consumer still passingclientgets no error — the option is simply not recognised and a default transport is built instead. Nothing in this repo passes either, and neitherIClientnor the field was re-exported from the package entry point, but a consuming app that injects its own client has to rename the option.default:fallthrough to POST is gone. The previousswitchfunnelled any unrecognised method into a POST; the method now goes straight tofetch. 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.AbortControllerwas left alone.Apibuilds one in its constructor and never replaces it, so onceabort()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/clearHeadersat the top of the file, which a livelogin/logoutfurther 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 explicitlynullpayload still throws where it always did.Steps to reproduce
key[]=params.customHeadersin a request's options; it still replaces the default headers for that request alone rather than merging with them.FormData; it is still passed tofetchunencoded.abort(); in-flight requests are still cancelled through the signalApithreads into every request.Tests
lib/api/__tests__/api.spec.tsis new — the REST layer had no coverage at all. It pins both sides of the new seam:key[]=params for arraysFormDatathrough unencodedcustomHeadersoption replace the headers entirelyabort()loggedIn()answers yes for an absent login, sinceevery()over no keys is vacuously trueignorepattern matching it