fix: Add domain to build_client

[grepfruit19]

This PR

• Adds domain lookup to build_client

Related Issues

Fixes #108

How to test

Instantiate the library with and without a domain for its provider.

[beeme1mr]

Could you please add some tests and update the readme?

@maxveldink, how would you feel about removing the name argument entirely? It would be a breaking change but help us avoid technical debut as we move towards a 1.0 release.

[grepfruit19]

@beeme1mr @maxveldink

I added some tests.

I didn't want to change the behavior of the library too much, but maybe trying to instantiate a client against a domain that doesn't exist should throw or something, I feel like instantiating with the NoOp provider is unexpected behavior.

[maxveldink]

Thanks for fixing this @grepfruit19! Definitely, an oversight I had when I was trying to convert us over from name to domain. I agree that we should completely drop name wherever it's used (pre-1.0, I don't think we should have the idea of "legacy" fields; we should attempt to match the spec as close as possible).

Further, I think dropping version makes sense as well. I think @technicalpickles or @josecolella could explain why it was added, but it's not a part of the current specification and might confuse users. One other thing to think through; I don't think we can reuse Metadata for both Providers and Clients; they have different required fields. I'd be up for either dropping that class in favor of a simple hash for now or introducing ClientMetadata and ProviderMetadata classes to explicitly state the keys we allow.

Let me know if that's enough to go on, or if you need more direction here. This will definitely be a breaking change.

[grepfruit19]

Sounds good, I can add the changes to drop name and version if we're in agreement. I'd rather keep this PR limited in scope as far as adding the ClientMetadata and ProviderMetadata classes however, so I'd prefer not to add those changes in this PR.

[grepfruit19]

@beeme1mr @maxveldink

I've removed name from the library, I think things should be good to go now. I've held off on removing version so that y'all can make a separate PR since it seems like that has some further discussion to be had.

Let me know if there's anything else to be done!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.40%. Comparing base (e8fb8cc) to head (6d92a63).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #109   +/-   ##
=======================================
  Coverage   99.39%   99.40%           
=======================================
  Files          12       12           
  Lines         166      167    +1     
=======================================
+ Hits          165      166    +1     
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[maxveldink]

We need to back out the changes to the Metadata#name field and keep that the same. The changes to build_client are good though 👍🏻

[maxveldink]

Going to wait a little while longer for others to review as well, will merge in by EoD

[beeme1mr]

Approved, but please double-check if exposing the provider is required before merging.

[grepfruit19]

@beeme1mr Is there a good way to check what provider a client is using then?

[beeme1mr]

@beeme1mr Is there a good way to check what provider a client is using then?

Hey @grepfruit19, you can use the provider metadata on the client. Here's how we implemented that in the JS SDK.

If you're interested, this is how we passed the provider reference to the client.

[grepfruit19]

@beeme1mr Then in terms of this ticket, would you be okay with me un-exposing the provider and holding off on exposing the provider metadata for future work?

[beeme1mr]

@beeme1mr Then in terms of this ticket, would you be okay with me un-exposing the provider and holding off on exposing the provider metadata for future work?

Sure, no problem at all. Thanks for your work on this PR.

[grepfruit19]

@beeme1mr I've updated the PR and commented out the corresponding tests

[grepfruit19]

@beeme1mr I think @maxveldink is out of office today, if this looks good to you mind giving it an approve + merge?