Skip to content
This repository has been archived by the owner on Dec 12, 2024. It is now read-only.

Remove protocol exports from http-client and http-server #52

Closed
wants to merge 4 commits into from

Conversation

KendallWeihe
Copy link
Contributor

Discussions from this PR w/ @leordev led us to the conclusion that http-client and http-server shouldn't export transitive abstractions; which is to say, abstractions from a different dependency

@mistermoe would this break anything? Open to push back on this topic

@changeset-bot
Copy link

changeset-bot bot commented Oct 19, 2023

🦋 Changeset detected

Latest commit: 6ea6b39

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 Major
@tbdex/http-server Major
@tbdex/protocol Major

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

@mistermoe
Copy link
Member

@KendallWeihe is there a recap of the rationale? motivation behind surfacing transitive abstractions was so that folks wouldn't have pull in more than 1 dependency - e.g. to use http-client you necessarily need protocol. it does seem a bit off though

@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023

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

Updated @ 2023-10-19T19:14:05.198Z - Commit: 995dfc5

@KendallWeihe
Copy link
Contributor Author

@KendallWeihe is there a recap of the rationale? motivation behind surfacing transitive abstractions was so that folks wouldn't have pull in more than 1 dependency - e.g. to use http-client you necessarily need protocol. it does seem a bit off though

Yeah and I think you and I talked about the "have to pull in more than 1 dependency" ... but it also opens ambiguity in terms of what abstractions live in which place.

This comment was the differentiator for me.

@mistermoe
Copy link
Member

it sounds like adding the transitive abstractions to peerDependencies might be a win/win

@KendallWeihe
Copy link
Contributor Author

it sounds like adding the transitive abstractions to peerDependencies might be a win/win

Yeah I think you're right. Going to work towards that... looks like I'll have to move a few things around in the code to do so. Stand by...

@KendallWeihe
Copy link
Contributor Author

looks like I'll have to move a few things around in the code to do so

Okay I see why http-server depends on http-client now... I don't love that, but I'm going to leave it as is for now. Because technically the shared code is specific to http... which would require a new package just @tbdex/http ... which we could do but it's unnecessary at this moment

@KendallWeihe
Copy link
Contributor Author

Alright @mistermoe @leordev can you look this over?

@KendallWeihe
Copy link
Contributor Author

I thought more about this and am fully bought in with this approach. The primary reason being Developer experience. Sure it's a slightly less good experience to have to install two packages. But, it makes the distinction between tbdex the transport agnostic protocol and the specific implementation over http much less ambiguous. Which is to say we're making it clear that tbdex isn't married to http.

@leordev
Copy link
Member

leordev commented Nov 15, 2023

@mistermoe @KendallWeihe gentle ping as a reminder!

@KendallWeihe
Copy link
Contributor Author

KendallWeihe commented Nov 15, 2023

I'll point out, that in our Kotlin implementations, it's like we have proposed here -- that is, a developer can't just install the httpclient package, but has to also install the protocol package


Edit: I'm not sure this is actually true, I'm not familiar enough w/ the java world, but I do know confidently that the import statements will still be from import tbdex.sdk.protocol.models even if the intent is to use the httpclient package

@jiyoonie9
Copy link
Contributor

@KendallWeihe is it ok to close this PR? it's been sitting for a bit.

@KendallWeihe
Copy link
Contributor Author

@KendallWeihe is it ok to close this PR? it's been sitting for a bit.

Heh, this is topical... my commentary here is on a PR in the js repo, but spans to all tbdex language SDK's

IMO this is still my vote for how we define dependencies -- via a peer dependency

This is a conceptually complex problem and there is no great solution, and this is true for all our language implementations, each with their own package management, build system & runtime quirks (I don't think Maven has this concept of peer dependencies whatsoever). Just to briefly talk it through... tbdex depends on web5, and the complexity I'm referring to comes into view with lining up compatible versions across both.

Say we didn't go with the peer dependency solution. And also, say we implemented the tbdex package wherein we totally abstract away from web5 (as in, the developer wouldn't need to import anything from the web5 package in order to use tbdex). So imagine in this situation tbdex is entirely self-contained without any abstraction leakage. But then say that a developer actually wants to install web5 for some reason outside-of their tbdex use case. At this point, the user has installed tbdex -- which contains a transitive dependency on web5 -- and they have also installed web5. There is a risk of version incompatibility at this stage. Effectively, there may be two distinct versions of web5 for which the project depends on.

Again, I'm speaking abstractly, outside the specifics of the npm ecosystem. Different package managers and different language runtimes will handle this in different ways. So let's take a step back and consider the DX.

The DX of a dependency being totally self-contained sounds so good to me, but also, I think it may be the type of thing which is seductive which comes back to bite us. Contrasted with what's being proposed here, which is that the developer must also install web5 if they want to use tbdex -- this is, on the surface, a worse DX because now the developer must take the extra step of considering version compatibility. "Okay in order to use tbdex I need to also install web5... oh but wait, which version of web5 should I install?"

I won't comment on the other languages, but npm's peer dependencies make this easy IMO. Because the specific version (& optionally a range) is associated to the given peer dependency which natively makes that available to the developer. And I get that we need to consider consistency across languages, but IMO this is one area where consistency is irrelevant b/c we're constrained by the given feature support in the specific languages' package manager, build system & runtime.

I still stand with the latter DX. Yes it adds more responsibility to the developer, but IMO that's an investment worth making.

Anyways... this is super long winded, sorry about that, but I need to make the case. I don't care if we want to close this PR, or merge it. If we close it then I'll open a ticket for us to battle it out in the future.

@KendallWeihe
Copy link
Contributor Author

Alright, this has zero traction. No benefit in keeping it around. Opened a ticket for further discussion #164

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants