Refactor http client #113

kirahsapong · 2023-12-07T03:23:50Z

Summary

This PR addresses #111

It adds custom error types including RequestError, ResponseError, ValidationError, custom validation error types including InvalidDidError and InvalidServiceEndpointError.

The http-client is refactored to throw a custom error type in case of a failure, and return data only in case of success.

This PR only introduces more narrow custom error types for ValidationErrors. We can track adding more narrow custom error types for request and response errors here.

changeset-bot · 2023-12-07T03:23:54Z

🦋 Changeset detected

Latest commit: 85e541a

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

This PR includes changesets to release 3 packages

Name	Type
@tbdex/http-client	Minor
@tbdex/http-server	Minor
@tbdex/protocol	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

KendallWeihe · 2023-12-07T15:05:45Z

packages/http-client/src/client.ts

+    try {
+      pfiServiceEndpoint = await TbdexHttpClient.getPfiServiceEndpoint(pfiDid)
+    } catch(e) {
+      throw new RequestError({ message: e['message'], recipientDid: pfiDid, cause: e })


Rather than catching and throwing the exception here, would it be more wise to throw the exception from within the TbdexHttpClient.getPfiServiceEndpoint function? This is the beauty of using exceptions for error cases, rather than the return type, in that we can bubble-up the exceptions throughout n-number of callstack layers

good catch yep!

KendallWeihe · 2023-12-07T15:05:58Z

Nice!

Embrace debate. I think it would be wise to be more fine-grained in our custom exceptions, with names that match the semantic failure. We're exchanging a design pattern where the return type has errors for using exceptions, which means the consuming developer is expected to key-off-of the given exceptions to drive their own business logic. By using a more-general exception, such as RequestError, the developer would have to do string-based conditionals based on the exceptions message. For example, this is how the current DX would look like...

try {
  await TbdexHttpClient.getOfferings({ pfiDid: 'hehetroll' })
} catch (e) {
  if (e.message.include('invalidDid')) { // this line
    console.log('This was an issue with the given DID')
  } else if (e.message.include('has no PFI service entry')) { // and this line
    console.log('This was a PFI discovery exception')
  }
}

But, if we fine-grained RequestError into two new exceptions, let's call them InvalidPfiDid and EmptyPfiServiceEndpoint (I think maybe we can do better on the names, just shooting from the hip), then the DX would look like this...

try {
  await TbdexHttpClient.getOfferings({ pfiDid: 'hehetroll' })
} catch (e) {
  if (e instanceof InvalidPfiDid) {
    console.log('This was an issue with the given DID')
  } else if (e instanceof EmptyPfiServiceEndpoint) {
    console.log('This was a PFI discovery exception')
  } 
}

With this approach, the general heuristic would be one-custom-exception-per-test-case

mistermoe · 2023-12-08T03:14:16Z

@kirahsapong we thinking about refactoring the rest of http-client in this PR or refactoring 1 method per PR?

mistermoe · 2023-12-08T03:15:28Z

@KendallWeihe could we address what you're bringing up by introducing a kind property to RequestError?

KendallWeihe · 2023-12-08T15:02:36Z

@KendallWeihe could we address what you're bringing up by introducing a kind property to RequestError?

Could, and think it would be fine, but my intuition is against it.

If we did, then we would want to define a type as a set of string literals, in the same way we do MessageKind. This way, we're still leveraging the static typing of TypeScript.

I think it's good for the DX, in that it would further add a layer of semantic reasoning to the developer of whether or not the given kind of exception was caused during the requesting or during the responding. But also, it's restrictive in that same sense in that certain kinds of exceptions may span both requesting and responding, in which case the additional hierarchy of categorization is useless overhead and only serves to confuse. I guess basically my point is, it's abstraction which doesn't offer net positive return.

I think it better to have a flattened set of exceptions, where the name of the exception is the valuable nugget of experience.

But, also, it's splitting hairs, so just do it one way or the other and it's fine 😆

kirahsapong · 2023-12-08T18:47:19Z

@mistermoe good point might be nice to do 1 per PR to keep the review a bit tighter

kirahsapong · 2023-12-08T19:01:50Z

@KendallWeihe agree with your reasoning!

kirahsapong · 2023-12-18T20:21:38Z

Added more info to the PR summary ✨

KendallWeihe

🎉

mistermoe · 2023-12-19T16:07:46Z

packages/http-client/src/errors/validation-error.ts

mistermoe · 2023-12-19T16:17:58Z

packages/http-client/src/client.ts

@@ -27,7 +28,7 @@ export class TbdexHttpClient {
   * @throws if recipient DID resolution fails
   * @throws if recipient DID does not have a PFI service entry
   */
-  static async sendMessage<T extends MessageKind>(opts: SendMessageOptions<T>): Promise<HttpResponse | ErrorResponse> {
+  static async sendMessage<T extends MessageKind>(opts: SendMessageOptions<T>): Promise<HttpResponse> {


thoughts on just returning Promise<void> here?

good catch thanks!

suppose we can remove DataResponse from our types, with a lil bit of tech debt to refactor our http-server tests to remove our HttpResponse and ErrorResponse in a follow up, yeah?

yup good idea!

github-actions · 2023-12-19T18:50:18Z

TBDocs Report

✅ No errors or warnings

@tbdex/protocol

Project entry file: packages/protocol/src/main.ts

@tbdex/http-client

Project entry file: packages/http-client/src/main.ts

@tbdex/http-server

Project entry file: packages/http-server/src/main.ts

TBDocs Report Updated at 2023-12-20T19:01:22Z 85e541a

sendMessage getOfferings getExchange getExchanges getPfiServiceEndpoint

…ndpoint

mistermoe · 2023-12-19T20:16:55Z

@kirahsapong as much as i want to use msw, i think it may be in our best interests to stub fetch instead. I've been working on updating how we run browser tests, but even with those changes, conditionally switching between the msw import for node vs. browser is going to be tricky.

Did you have an approach in mind for conditionally switching between import { setupWorker } from 'msw/browser' and import { setupServer } from 'msw/node'?

Otherwise, i think we can follow this example from web5-js to mock fetch!

kirahsapong · 2023-12-19T20:19:45Z

@kirahsapong as much as i want to use msw, i think it may be in our best interests to stub fetch instead. I've been working on updating how we run browser tests, but even with those changes, conditionally switching between the msw import for node vs. browser is going to be tricky.

Did you have an approach in mind for conditionally switching between import { setupWorker } from 'msw/browser' and import { setupServer } from 'msw/node'?

Otherwise, i think we can follow this example from web5-js to mock fetch!

yeah have the same concern! my initial instinct is the overhead to switch betwixt will outweigh any benefit when all we're using msw for is stubbing fetch. i'm good to switcheroo!

phoebe-lew

nice nice, can cut a new release 🚀

phoebe-lew · 2023-12-20T06:36:11Z

packages/http-client/src/client.ts

-        headers : response.headers,
-        errors  : responseBody.errors
-      }
+    if (!response.ok) {


so if this function returns nothing, that means the response was ok?

mistermoe · 2023-12-20T07:22:38Z

oh yeah! last thing to note would be that we probably should add a changeset to trigger a version bump. we should highlight that this is a breaking change and send the changelog over to devrel incase they've built any examples or written guides using TbdexHttpClient

* main: Remove HttpResponse and ErrorResponse from `http-client` types (#124) Refactor http client to introduce custom error types (#113) Finish validating rfq against provided offering (#120) Generates the HTML to publish to GH Pages (#116) http-server fixes + some tests (#83)

kirahsapong requested review from michaelneale, mistermoe, jiyoonie9, phoebe-lew and KendallWeihe as code owners December 7, 2023 03:23

kirahsapong requested a review from diehuxx December 7, 2023 03:24

kirahsapong mentioned this pull request Dec 7, 2023

Refactor http-client #111

Closed

KendallWeihe reviewed Dec 7, 2023

View reviewed changes

kirahsapong force-pushed the origin/refactor-http-client branch from bdfeb27 to 60e6ecb Compare December 7, 2023 17:16

kirahsapong marked this pull request as draft December 8, 2023 19:01

kirahsapong force-pushed the origin/refactor-http-client branch from 7851abe to 056b044 Compare December 18, 2023 19:30

kirahsapong marked this pull request as ready for review December 18, 2023 19:56

kirahsapong mentioned this pull request Dec 18, 2023

Extend custom error types for http-client #117

Open

kirahsapong requested a review from KendallWeihe December 18, 2023 20:14

KendallWeihe approved these changes Dec 19, 2023

View reviewed changes

mistermoe reviewed Dec 19, 2023

View reviewed changes

packages/http-client/src/errors/validation-error.ts Outdated

Copy link

Member

mistermoe Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

mistermoe reviewed Dec 19, 2023

View reviewed changes

mistermoe and others added 4 commits December 19, 2023 12:07

start refactor

d65b472

add deps

74e2bb7

Add msw and more tests

b58e0f6

Add todo comment

a73e8e0

kirahsapong added 12 commits December 19, 2023 12:07

fix deps

bbd420b

Add validation errors

c7470a7

Add sinon pkg

1642ac0

Refactor client

d9b47f8

sendMessage getOfferings getExchange getExchanges getPfiServiceEndpoint

Add mocks and tests for sendMessage, getOfferings, and getPfiServiceE…

e49885e

…ndpoint

Add tests for getExchange and getExchanges

dc4e352

small revert

9397438

small import fix

69f703e

small revert

e5302f3

remove httpresponse return val from sendmessage

42f729b

Rename InvalidServiceEndpointError to MissingServiceEndpointError

70a617f

Remove DataResponse type

8153e4a

kirahsapong force-pushed the origin/refactor-http-client branch from ad7978a to 8153e4a Compare December 19, 2023 20:08

kirahsapong added 2 commits December 19, 2023 13:28

Remove msw in favour of sinon stub

d0858ca

Remove msw pkg

0f56b70

kirahsapong requested a review from mistermoe December 19, 2023 21:32

phoebe-lew approved these changes Dec 20, 2023

View reviewed changes

This was referenced Dec 20, 2023

Refactor http-server tests to remove ErrorResponse #123

Closed

Document custom exception types TBD54566975/tbdex#211

Closed

kirahsapong added 2 commits December 20, 2023 10:58

Add changeset

02a9006

Update changeset

85e541a

kirahsapong merged commit 9e1015e into main Dec 20, 2023
7 checks passed

kirahsapong deleted the origin/refactor-http-client branch December 20, 2023 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor http client #113

Refactor http client #113

kirahsapong commented Dec 7, 2023 •

edited

Loading

changeset-bot bot commented Dec 7, 2023 •

edited

Loading

KendallWeihe Dec 7, 2023

kirahsapong Dec 8, 2023

KendallWeihe commented Dec 7, 2023

mistermoe commented Dec 8, 2023

mistermoe commented Dec 8, 2023

KendallWeihe commented Dec 8, 2023

kirahsapong commented Dec 8, 2023

kirahsapong commented Dec 8, 2023

kirahsapong commented Dec 18, 2023

KendallWeihe left a comment

mistermoe Dec 19, 2023

mistermoe Dec 19, 2023

kirahsapong Dec 19, 2023

mistermoe Dec 19, 2023

github-actions bot commented Dec 19, 2023 •

edited

Loading

mistermoe commented Dec 19, 2023

kirahsapong commented Dec 19, 2023

phoebe-lew left a comment

phoebe-lew Dec 20, 2023

mistermoe Dec 20, 2023

mistermoe commented Dec 20, 2023

Refactor http client #113

Refactor http client #113

Conversation

kirahsapong commented Dec 7, 2023 • edited Loading

Summary

changeset-bot bot commented Dec 7, 2023 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KendallWeihe commented Dec 7, 2023

mistermoe commented Dec 8, 2023

mistermoe commented Dec 8, 2023

KendallWeihe commented Dec 8, 2023

kirahsapong commented Dec 8, 2023

kirahsapong commented Dec 8, 2023

kirahsapong commented Dec 18, 2023

KendallWeihe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 19, 2023 • edited Loading

@tbdex/protocol

@tbdex/http-client

@tbdex/http-server

mistermoe commented Dec 19, 2023

kirahsapong commented Dec 19, 2023

phoebe-lew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistermoe commented Dec 20, 2023

kirahsapong commented Dec 7, 2023 •

edited

Loading

changeset-bot bot commented Dec 7, 2023 •

edited

Loading

github-actions bot commented Dec 19, 2023 •

edited

Loading