Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scoped clients #9

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

lukeredpath
Copy link

@lukeredpath lukeredpath commented May 10, 2022

Adds a new .scoped operator on URLRoutingClient that can be used to create scoped clients.

This supports a use case where your main client may support a large number of routes and you only want to be able to pass around a client that handles a sub-set of those routes as a dependency.

For instance, imagine a feature domain within a TCA app - your AppEnvironment may hold onto a URLRoutingClient<AppRoute> client but your account feature may only need to make requests to account-related routes, so you'd like to pass a URLRoutingClient<AccountRoute> client to that domain's environment, limiting its scope and making it easier for you to find the right route at the call-site.

I debated over whether or not to call this scoped or pullback - I think the shape of this function is a pullback but the term "scope" or "scoped" felt more ergonomic to me in this context.

Also added some basic tests for the client.

Suggestion - consider adding some integration tests of the live implementation using a mock server. There's a few available (I've found https://github.com/surpher/PactSwift really easy to use although its use cases extend beyond simply creating a mock server).

@davdroman
Copy link

This is great. I followed your thought process in your other thread and I think you're onto something very important here.

As a side note, I think it might be good to drop the "d" and call it scope so it matches TCA's nomenclature. What do you think?

@lukeredpath
Copy link
Author

@davdroman for consistency you might be right, but to me scope reads like its mutating the receiver, where as scoped implies that it's returning a new client (which it does).

@stephencelis
Copy link
Member

Hey @lukeredpath! Thanks for the PR.

We can theoretically see this being useful but would love to take it for a spin, ideally both in our episode demo and a larger project like isowords, to see how it can help. We briefly explored a variant of scoping while developing the library, but found that our solution wasn't useful on pointfree.co because our features rely so much on cross-linking to other parts of the site.

If you've used this method in a real world code base, we'd also love to see more examples and/or hear more about how it's worked and improved things, as well as any caveats you encountered!

@kamcma
Copy link

kamcma commented May 1, 2023

I did try using this in a project, and wanted to share one small wrinkle I found, in case this were ever picked up again.

I wanted to do something like this:

import Dependencies
import URLRouting

extension URLRoutingClient<ApiRoute>: TestDependencyKey {
  public static let testValue = Self.failing
}

extension DependencyValues {
  public var apiClient: URLRoutingClient<ApiRoute> {
    get { self[URLRoutingClient<ApiRoute>.self] }
    set { self[URLRoutingClient<ApiRoute>.self] = newValue }
  }
}

extension URLRoutingClient<ApiRoute> {
  public var books: URLRoutingClient<BooksRoute> {
    .init { booksRoute in
      try await self.data(for: .books(booksRoute))
    }
  }
}

so that I could then use it like this, as a means of minimizing what routes a feature is requesting access to:

@Dependency(\.apiClient.books) var booksClient

However, I quickly realized my computed client, and I think the scope implementation in this PR, is not passing along the URLRoutingClient's configured JSONDecoder. Which I think you'd really want to do, and not hard-code a custom decoder in every extension, so that it remains configurable from the top level.

However, URLRoutingClient holds on to that decoder internally, so I think this kind of scoping is currently not possible in an extension.

So, either a shipped .scoped operator would want to pass along the decoder, or perhaps the decoder could be made public, so this kind of scoping would be possible ad-hoc via extension.

@lukeredpath
Copy link
Author

lukeredpath commented May 1, 2023

@kamcma what do you mean by configured decoder? I don’t see any decoder in the client, except for the one you pass in when calling the particular request overload that takes one. The entire implementation of the client is encapsulated in the request closure that the scoped overload delegates to.

Edit: oh I see what you’re referring to now. You are looking at the latest implementation in main. There was no internal decoder when I opened this PR.

@kamcma
Copy link

kamcma commented May 1, 2023

Ah, I apologize for framing it as an omission. That possibility had not occurred to me.

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

Successfully merging this pull request may close these issues.

None yet

4 participants