Simplify types, inheritance structure, and API #156

diehuxx · 2024-02-03T09:55:48Z

All Changes

Simplify inheritance structure of Message and its subclasses and Resource and its subclasses.
Simplify types in protocol and http-server packages. Remove generics wherever possible. Replace generics with intersection types which are easier to read. Remove all types which could be considered "typescript neckbeardery"
Remove Message.factory and Resource.factory. Introduce standalone functions parseMessage and parseResource, reducing coupling and circular dependency.
Improve test coverage tests for Message and Rfq. In follow up PRs, I will further improve test coverage in the all three packages. I'm not testing in this PR because it's already a big diff.
Slight refactor to the API of TbdexHttpServer for adding callback functions. http-server has no tests currently, so tests for this new API will come in a follow up PR.

Changes to public API surface

tbdex/protocol

I updated the README to reflect the new uses.

All uses of MessageKindClass and ResourceKindClass have been replaced with Message and Resource.

Message.parse and Resource.parse have been replaced with parseMessage() and parseResource().

Each message-kind subclass and resource-kind subclass (e.g. Rfq, Offering, etc.) now have their own dedicated .parse() method which checks that the input is the expected message kind. For example, Rfq.parse(inputJson) will return an instance of the Rfq class if and only if inputJson is a valid, signed RFQ.

Message.verify(), Resource.verify(), Message.validate(), and Resource.validate() are no longer static methods. They have been replaced with instance methods

tbdex/http-server

Updated to reflect type changes from tbdex/protocol, most notably MessageKindClass -> Message.

Alter API for adding callbacks methods to Tbdex server. Removes methods get and submit, replacing them with

onSubmitRfq(callback: SubmitRfqCallback): void
onSubmitOrder(callback: SubmitOrderCallback): void
onSubmitClose(callback: SubmitCloseCallback): void
onGetExchanges(callback: GetExchangesCallback): void
onGetOfferings(callback: GetOfferingsCallback): void

changeset-bot · 2024-02-03T09:55:51Z

🦋 Changeset detected

Latest commit: 099c88f

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

github-actions · 2024-02-03T10:06:17Z

TBDocs Report

🛑 Errors: 0
⚠️ Warnings: 12

@tbdex/protocol

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

@tbdex/http-client

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

📄 File: packages/http-client/src/errors/request-error.ts
⚠️ `extractor:ae-missing-release-tag`: "RequestErrorParams" is part of the package's API, but it is missing a release tag (`@alpha`, `@beta`, `@public`, or `@internal`) #L1
⚠️ `extractor:ae-undocumented`: Missing documentation for "RequestErrorParams". #L1
⚠️ `extractor:ae-undocumented`: Missing documentation for "recipientDid". #L13
⚠️ `extractor:ae-undocumented`: Missing documentation for "url". #L14

📄 File: packages/http-client/src/errors/request-token-error.ts
⚠️ `extractor:ae-missing-release-tag`: "RequestTokenErrorParams" is part of the package's API, but it is missing a release tag (`@alpha`, `@beta`, `@public`, or `@internal`) #L3
⚠️ `extractor:ae-undocumented`: Missing documentation for "RequestTokenErrorParams". #L3

📄 File: packages/http-client/src/errors/response-error.ts
⚠️ `extractor:ae-missing-release-tag`: "ResponseErrorParams" is part of the package's API, but it is missing a release tag (`@alpha`, `@beta`, `@public`, or `@internal`) #L3
⚠️ `extractor:ae-undocumented`: Missing documentation for "ResponseErrorParams". #L3
⚠️ `extractor:ae-undocumented`: Missing documentation for "statusCode". #L15
⚠️ `extractor:ae-undocumented`: Missing documentation for "details". #L16
⚠️ `extractor:ae-undocumented`: Missing documentation for "recipientDid". #L17
⚠️ `extractor:ae-undocumented`: Missing documentation for "url". #L18

@tbdex/http-server

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

TBDocs Report Updated at 2024-02-06T23:33:14Z 099c88f

mistermoe · 2024-02-04T19:42:00Z

diving into this now

packages/protocol/tests/offering.spec.ts

mistermoe · 2024-02-04T21:37:20Z

regarding changes made to `@tbdex/protocol`

Definitely feels like an overall improvement! shaved the neckbeard at the cost of what looks like repetition on initial glance but i think could be argued isn't actually. Either way, the cognitive overhead required to add a new message kind or make a change is significantly reduced which is awesome! Just compared adding a new message kind on main and then on your branch and it's significantly easier here.

Also, it doesn't seem like we sacrificed any type specificity at the public api layer. VSCode is still correctly guiding me through creating all types of messages. Biggest wins imo are:

ditching MessageKind which was super annoying because it was difficult to decide whether to Message or MessageKind.
ditching the hack i added to prevent circular dependencies

regarding changes made to `@tbdex/http-server`:

Still forming an opinion here because the api surface did notably change.

Other comments:

Can you include a writeup of all the breaking changes at the public api level with examples so that we can share it with devrel? i think they have guides that are using what's currently on main and we'll need a way to convey what's changed and how.

diehuxx · 2024-02-05T07:12:10Z

Definitely feels like an overall improvement! shaved the neckbeard at the cost of what looks like repetition on initial glance but i think could be argued isn't actually.

Glad you think so! I made a deliberate choice to allow small amounts of repetition in order to make the API surface and the implementation simpler.

Can you include a writeup of all the breaking changes at the public api level with examples so that we can share it with devrel? i think they have guides that are using what's currently on main and we'll need a way to convey what's changed and how.

Absolutely. I'll add details to the PR description tomorrow.

codecov · 2024-02-05T07:12:48Z

Codecov Report

Merging #156 (654c31f) into main (85e5841) will increase coverage by 6.82%.
The diff coverage is 91.27%.

❗ Current head 654c31f differs from pull request most recent head 099c88f. Consider uploading reports for the commit 099c88f to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
+ Coverage   78.69%   85.52%   +6.82%     
==========================================
  Files          19       35      +16     
  Lines        1169     2798    +1629     
  Branches       99      234     +135     
==========================================
+ Hits          920     2393    +1473     
- Misses        249      405     +156

Components	Coverage Δ
protocol	`90.01% <95.49%> (∅)`
http-client	`86.78% <78.04%> (-0.05%)`	⬇️
http-server	`71.30% <70.96%> (+1.17%)`	⬆️

* main: adding `replyTo` field in http request body to submit RFQ (#142)

jiyoonie9 · 2024-02-06T15:54:42Z

packages/http-client/src/client.ts

-      data.push(resource)
+    const responseBody = await response.json()
+    const jsonOfferings = responseBody.data as any[]
+    for (let jsonResource of jsonOfferings) {


Suggested change

for (let jsonResource of jsonOfferings) {

for (let jsonOffering of jsonOfferings) {

jiyoonie9

soo awesome!! i love the changes you've made to protocol (which in turn also affects http-client positively in simplifying types and tightening type constraints wherever possible). i also very much appreciate the small cleanups throughout this PR.

i don't immediately love the api changes to http-server - i wish we could preserve some of our generic magic in submit<T> and get<T> so we don't have to write / update concrete impls if tbdex messages change in the future. but i think changes i have to make as a consumer won't be too bad.

jiyoonie9 · 2024-02-06T15:56:45Z

packages/http-client/src/client.ts

  /** the message you want to send */
-  message: Message<T> | MessageModel<T>
+  message: T


this is pretty great!!

packages/http-client/src/client.ts

packages/http-server/src/http-server.ts

jiyoonie9 · 2024-02-06T16:10:23Z

packages/protocol/package.json

@@ -82,7 +83,7 @@
    "build:esm": "rimraf dist/esm dist/types && tsc",
    "build:cjs": "rimraf dist/cjs && tsc -p tsconfig.cjs.json && echo '{\"type\": \"commonjs\"}' > ./dist/cjs/package.json",
    "build:browser": "rimraf dist/browser.mjs dist/browser.js && node build/bundles.js",
-    "test:node": "rimraf tests/compiled && pnpm compile-validators && tsc -p tests/tsconfig.json && mocha",
+    "test:node": "rimraf tests/compiled && pnpm compile-validators && tsc -p tests/tsconfig.json && c8 mocha",


what is c8 for?

c8 produces a code coverage report when you run the tests

jiyoonie9 · 2024-02-06T16:12:23Z

packages/protocol/src/message-kinds/rfq.ts

      id         : id,
      exchangeId : id,
      createdAt  : new Date().toISOString()
    }

-    // TODO: hash `data.payinMethod.paymentDetails` and set `private`


could we keep these // TODO if they aren't completed in this pr and are still to be done?

looks like in other areas of this PR that we're doing away with the concept of private fields? not 100% sure tho

My bad, I should have provided more context there. In this proposal for how to handle private data, we're doing away with the notion of private as a top-level field in RFQs. This PR does not implement the proposal, but it does clean up private in preparation for implementing the proposal. I'll put the TODO back in.

packages/protocol/src/message-kinds/rfq.ts

jiyoonie9 · 2024-02-06T16:24:34Z

packages/protocol/src/parse.ts

+  let message: Message
+  switch(jsonMessage.metadata.kind) {
+    case 'rfq':
+      message = new Rfq(


Suggested change

message = new Rfq(

message = await Rfq.parse(rawMessage)

just wondering if we can use parse() method you already wrote for each of the concrete message impls?

I'd prefer not to reuse Rfq.parse etc. Rfq.parse() does some of the same things that are already done in parseMessage, so we would be repeating operations (.verify(), and rawToMessageModel). While we could remove those operations from parseMessage, I think it would be less clear what parseMessage is doing. Also, i prefer to think of Rfq.parse() and parseMessage as part of the public API for this library, and that we use the constructors and instance methods when appropriate internally.

jiyoonie9 · 2024-02-06T16:39:24Z

packages/protocol/tests/offering.spec.ts

    })
  })

+  // describe('validate', () => {


remove these commented out bits if not relevant anymore?

jiyoonie9 · 2024-02-06T16:43:32Z

packages/protocol/src/parse.ts

+ * @param message - the message to parse. can either be an object or a string
+ * @returns {@link Message}
+ */
+export async function parseMessage(rawMessage: MessageModel | string): Promise<Message> {


thoughts on being able to call this like MessageUtils.parseMessage() as a static method? a good amount of our other API surfaces tend to follow this pattern i.e. Rfq.parse() or Crypto.verify()

Works for me. Instead of MessageUtils, I'm calling it Parser which matches the grammatical pattern of our Validator and DidResolver. MessageUtils seems a little too broad, plus this file contains utils for parsing resources in addition to messages.

jiyoonie9 · 2024-02-06T16:55:08Z

oh, could you add a changeset? steps here

diehuxx · 2024-02-06T22:36:57Z

i don't immediately love the api changes to http-server - i wish we could preserve some of our generic magic in submit and get

Fair enough. Here's my defense.

First, a petite polemic: Generics are a fatal temptation. The DRYness that generics promise only breeds a thirst for even more generics. One drop may be invigorating, but we must not become addicted, lest we resent their absence.

In this PR I adopt the principle that anything that can be accomplished clearly and readably without generics, should be done without generics. The existing get and submit functions in http-server have largely the same appearance as the new onSubmitRfq(), onGetOfferings, etc.

For example, server.submit('rfq', rfqCallback) looks pretty similar to server.onSubmitRfq(rfqCallback). Both versions require the consumer to realize that they are specifying a callback for the submit RFQ endpoint. But the latter has a simpler implementation by avoiding the use of generics. As a bonus, I'd argue that it's easier to document/understand that calling server.onSubmitRfq(rfqCallback) overwrites (rather than adds to) the original callback.

so we don't have to write / update concrete impls if tbdex messages change

I'd argue this is an anti-pattern in the context of tbdex/http-server. There's a limited number of endpoints we're trying to support. If we add new tbdex messages and endpoints, we should have to add new methods to TbdexHttpServer and add unit tests accordingly. Being able to add new endpoints without needing to update TbdexHttpServer seems like a recipe for untested codepaths.

mistermoe · 2024-02-06T23:14:53Z

fair @diehuxx i'll take maintainability and clarity over hipster api surface vibes. fwiw server.submit('rfq', rfqCallback) always felt kinda odd to me when i implemented it because i always read it as "i the server am submitting an rfq and then calling rfqCallback". whereas server.onSubmitRfq reads far more like "i the server, on receiving an rfq will call rfqCallback

looks like all that's left is addressing the tbdocs warnings i think?

mistermoe · 2024-02-06T23:17:35Z

cc: @angiejones @chris-tbd

diehuxx requested review from michaelneale, mistermoe, jiyoonie9, phoebe-lew, KendallWeihe and kirahsapong as code owners February 3, 2024 09:55

Simplify types, inheritance structure, and API

c77d9b4

diehuxx force-pushed the simplify-types branch from e6a58ba to c77d9b4 Compare February 3, 2024 10:04

mistermoe reviewed Feb 4, 2024

View reviewed changes

packages/protocol/tests/offering.spec.ts Outdated Show resolved Hide resolved

mistermoe reviewed Feb 4, 2024

View reviewed changes

packages/protocol/tests/offering.spec.ts Show resolved Hide resolved

Lint

ecab9d7

Diane Huxley added 9 commits February 5, 2024 12:19

Merge branch 'main' into simplify-types

e406f8e

* main: adding `replyTo` field in http request body to submit RFQ (#142)

Update submodule to latest main

c35fa03

Increase documentation coverage in protocol package

7b6c846

Fix tsdocs inheritdoc

707d5f3

typoooo

3eaa8d5

Remove rawToMessageModel and rawToResourceModel from public API

6333872

Fix capitalization on inheritDoc

8f20432

Give up on inheritDoc

298a90e

Resolve link doc tag warnings

0e86cf3

jiyoonie9 reviewed Feb 6, 2024

View reviewed changes

jiyoonie9 approved these changes Feb 6, 2024

View reviewed changes

@jiyoontbd PR comments

6a3daab

Add changeset

16e8880

diehuxx force-pushed the simplify-types branch from 32a8777 to 483dc1d Compare February 6, 2024 22:00

Document raw to model functions

654c31f

diehuxx force-pushed the simplify-types branch from 483dc1d to 654c31f Compare February 6, 2024 22:02

mistermoe approved these changes Feb 6, 2024

View reviewed changes

Address tbdocs warnings for http-server

099c88f

diehuxx merged commit 1b48ad1 into main Feb 6, 2024
9 checks passed

diehuxx deleted the simplify-types branch February 6, 2024 23:33

jiyoonie9 mentioned this pull request Feb 16, 2024

Examine Message types and simplify if possible. Simplify http-server callbackmaps TBD54566975/tbdex-kt#172

Open

diehuxx mentioned this pull request Feb 26, 2024

Split TbdexHttpClient.sendMessage() into separate methods createExchange(), submitOrder(), and submitClose() #187

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify types, inheritance structure, and API #156

Simplify types, inheritance structure, and API #156

diehuxx commented Feb 3, 2024 •

edited

Loading

changeset-bot bot commented Feb 3, 2024 •

edited

Loading

github-actions bot commented Feb 3, 2024 •

edited

Loading

mistermoe commented Feb 4, 2024

mistermoe commented Feb 4, 2024

diehuxx commented Feb 5, 2024

codecov bot commented Feb 5, 2024 •

edited

Loading

jiyoonie9 Feb 6, 2024

jiyoonie9 left a comment

jiyoonie9 Feb 6, 2024

jiyoonie9 Feb 6, 2024

diehuxx Feb 6, 2024

jiyoonie9 Feb 6, 2024

jiyoonie9 Feb 6, 2024

diehuxx Feb 6, 2024

diehuxx Feb 6, 2024

jiyoonie9 Feb 6, 2024

diehuxx Feb 6, 2024 •

edited

Loading

jiyoonie9 Feb 6, 2024

diehuxx Feb 6, 2024

jiyoonie9 Feb 6, 2024

diehuxx Feb 6, 2024

jiyoonie9 commented Feb 6, 2024

diehuxx commented Feb 6, 2024 •

edited

Loading

mistermoe commented Feb 6, 2024

mistermoe commented Feb 6, 2024

	for (let jsonResource of jsonOfferings) {
	for (let jsonOffering of jsonOfferings) {

Simplify types, inheritance structure, and API #156

Simplify types, inheritance structure, and API #156

Conversation

diehuxx commented Feb 3, 2024 • edited Loading

All Changes

Changes to public API surface

tbdex/protocol

tbdex/http-server

changeset-bot bot commented Feb 3, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Feb 3, 2024 • edited Loading

@tbdex/protocol

@tbdex/http-client

@tbdex/http-server

mistermoe commented Feb 4, 2024

mistermoe commented Feb 4, 2024

regarding changes made to @tbdex/protocol

regarding changes made to @tbdex/http-server:

Other comments:

diehuxx commented Feb 5, 2024

codecov bot commented Feb 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

jiyoonie9 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

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

diehuxx Feb 6, 2024 • edited Loading

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

jiyoonie9 commented Feb 6, 2024

diehuxx commented Feb 6, 2024 • edited Loading

mistermoe commented Feb 6, 2024

mistermoe commented Feb 6, 2024

diehuxx commented Feb 3, 2024 •

edited

Loading

changeset-bot bot commented Feb 3, 2024 •

edited

Loading

github-actions bot commented Feb 3, 2024 •

edited

Loading

regarding changes made to `@tbdex/protocol`

regarding changes made to `@tbdex/http-server`:

codecov bot commented Feb 5, 2024 •

edited

Loading

diehuxx Feb 6, 2024 •

edited

Loading

diehuxx commented Feb 6, 2024 •

edited

Loading