Stricten, test, and bugfix http-server #170
Conversation
|
diehuxx
force-pushed
the
ts-strict-http-server
branch
from
6ae9c0a to
e60d7ae
Compare
|
TBDocs Report 🛑 Errors: 0 @tbdex/protocol
@tbdex/http-client
@tbdex/http-server
TBDocs Report Updated at 2024-02-16T22:41:57Z |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #170 +/- ##
==========================================
+ Coverage 88.99% 92.48% +3.49%
==========================================
Files 36 37 +1
Lines 2825 2995 +170
Branches 262 321 +59
==========================================
+ Hits 2514 2770 +256
+ Misses 311 225 -86
|
diehuxx
requested review from
michaelneale,
mistermoe,
jiyoonie9,
phoebe-lew,
KendallWeihe and
kirahsapong
as code owners
|
I'm deliberately not adding a changeset to this PR since I don't want to release a new version at this point. This PR adds a lot of tests and cleanup, but there is still more to come immediately afterward. |
|
cc: @angiejones |
|
This all makes sense to me (I only briefly glossed over the tests)
I think it may be good to list any breaking changes (changes to function signatures -- I call these "binary incompatibilities"), even if it's here in GitHub. But also, I'm not trying to promote unnecessary toil so I'm supportive of whatever we think is best. We have to manually cut releases so it's not like merging this will immediately break anything. Great work! |
|
yooo @diehuxx
what was the issue? |
| offeringsApi: OfferingsApi | ||
| exchangesApi: ExchangesApi | ||
| } | ||
|
|
||
| export function createExchange(options: CreateExchangeOpts): RequestHandler { | ||
| export async function createExchange(req: Request, res: Response, options: CreateExchangeOpts): Promise<any> { |
There was a problem hiding this comment.
could you explain the change from returning RequestHandler to Promise<any> ? i also am noticing that req and res are included as params as opposed to being abstracted away in the RequestHandler type definition
There was a problem hiding this comment.
I just pushed a commit changing the return type of this function to Promise<void>, explanation in this comment thread.
We're not longer using RequestHandler because the usage of this function has changed (see this comment for details). Previously, createExchange would return a function, which would be consumed by the express.js router like this:
this.api.post('/exchanges/:exchangeId/rfq', createExchange({
callback: this.callbacks['rfq'], offeringsApi, exchangesApi,
}))Now, createExchange does NOT return a function. It is consumed as a curried function by the express js router like this
this.api.post('/exchanges/:exchangeId/rfq', (req: Request, res: Response) =>
createExchange(req, res, {
callback: this.callbacks['rfq'],
offeringsApi,
exchangesApi,
})| return async function (request, response) { | ||
| const queryParams = request.query as GetOfferingsFilter | ||
| const offerings = await offeringsApi.getOfferings({ filter: queryParams || {} }) | ||
| const filter: GetOfferingsFilter = { |
There was a problem hiding this comment.
i like this explicit typing, but was request.query as GetOfferingsFilter not working as expected?
There was a problem hiding this comment.
Using as in typescript is like telling the compiler "just trust me bro", so it's good to avoid it unless we as programmers are pretty confident. Since request.query could realistically be anything, it's better to select the specific fields we want and use filter: GetOfferingsFilter to help us remember which fields we need.
TL;DR functional programming is confusing. Previously, in the this.api.get('/offerings', getOfferings({
callback: this.callbacks['offerings'], offeringsApi
}))This calls this.api.get('/offerings', (req, res) =>
getOfferings(req, res, {
callback: this.callbacks['offerings'],
offeringsApi
})
)The arrow method const api = new TbdexHttpServer()
const server = api.listen()
api.onSubmitRfq(() => console.log('We received an RFQ'))
// When an RFQ is submitted, it prints
// "We received an RFQ"
// We can also overwrite the old callback
api.onSubmitRfq(() => console.log('TBDex is my best friend'))
// When an RFQ is submitted, it prints
// "TBDex is my best friend" |
| import { InMemoryExchangesApi } from '../src/in-memory-exchanges-api.js' | ||
| import { InMemoryOfferingsApi } from '../src/in-memory-offerings-api.js' | ||
| import { PortableDid } from '@web5/dids' | ||
| import Sinon from 'sinon' | ||
|
|
||
| describe('POST /exchanges/:exchangeId/rfq', () => { |
There was a problem hiding this comment.
i feel like we have a race condition happening here as it fails occasionally but maybe im gaslighting myself
There was a problem hiding this comment.
Which test(s) specifically are failing? I haven't seen any flakiness but I can investigate if you let me know where to look.
diehuxx
force-pushed
the
ts-strict-http-server
branch
from
f5bfd05 to
ab0bc39
Compare
Co-authored-by: Jiyoon Koo <[email protected]>
All Changes
strictmode for thehttp-serverpackage. Fix any bugs thatstrictmode reveals.callbackedge cases, which will be reworked in a follow up PR.TbdexHttpServercalls thecallbacksbecause the previous way actually did not work.fakes.ts. Flesh out the "fake" implementations ofExchangesApiandOfferingsApias fully usableInMemoryimplementations. Seein-memory-exchanges-api.tsandin-memory-offerings-api.ts. These do not yet have 100% test coverage simply because this PR already adds ~1200 lines of tests; I'll add more tests in a follow up PR.Changes to external API
Add fully fleshed out
InMemoryExchangesApiandInMemoryOfferingsApi. These are used as defaults inTbdexHttpServer, but may be worth documenting in their own right.Items TODO in follow up PRs
in-memory-exchanges-api.tsandin-memory-offerings-api.ts.