Context

[pepicrft]

Context

Despite XCTest and Xcode supporting running tests in parallel, we can't because our integration and acceptance tests can't be fully isolated, and increasing parallelism would translate into flakiness for our test suite. We need to do something to benefit as much as we can from the environment in which tests run (i.e., fast multi-core environments).

What

I'm adding a new class Context, which should eventually be an Actor, that will provide an interface to interact with the outside world (network, environment variables, file system), and that we can inject all the way down from commands. Note that this will increase the verbosity of function definitions a bit, but it'll give us the flexibility to parallelize all our test suite.

Note that I continue using a Context.shared instance in most of the places to prevent this PR from becoming huge and taking the discussion away from the concept of context and the role it'll play in the codebase. Once we all agree on the shape and the approach, I can open a follow-up PR getting rid if the .shared.

Alternatives

I discarded continuing down the path that we had followed from the beginning for the aforementioned reasons. Another alternative that I considered with the aim of mitigating some of the verbosity that our functions will end up with, is using some kind of dependency-injection framework. However, that'd create a strong dependency on external tooling or libraries, or even worse, some build-time tooling that generates the dependency-injection code. I believe the closer we stay to the language capabilities and paradigms, the better. I wouldn't be surprised if Apple releases something in that area, so if that ever happens, it's better not having to peel layers of abstractions to benefit from Apple's APIs.

[danieleformichelli]

Haven't gone through the whole PR but I have 2 questions:

1. Why are we passing the context to the functions, rather than to the inits?
2. Have we considered using https://github.com/pointfreeco/swift-dependencies ? I have never used it outside of TCA, but I have been very happy with it

[pepicrft]

1. Why are we passing the context to the functions, rather than to the inits?

Dependency injection through constructors is something that we embraced in various parts and that led to
the proliferation of factories throughout the codebase to give tests the flexibility to replace instances that take all the parameters via constructors. Some examples are: CacheDirectoriesProviderFactory, GraphMapperFactory, GeneratorFactory... The emergence of factories as a tool to facilitate testing is a code smell and this is a solution for that.

Have we considered using https://github.com/pointfreeco/swift-dependencies ? I have never used it outside of TCA, but I have been very happy with it

I touched on this in the description of the issue. I think this is a solvable problem with Swift and its primitives. Dependencies have a cost that we'd need to pay, and we've already done a fair amount of work learning from past mistakes in this area (i.e., introducing non-conventional patterns and strong dependencies on additional tools/patterns). So I'd avoid it.

[fortmarek]

Dependency injection through constructors is something that we embraced in various parts and that led to
the proliferation of factories throughout the codebase to give tests the flexibility to replace instances that take all the parameters via constructors. Some examples are: CacheDirectoriesProviderFactory, GraphMapperFactory, GeneratorFactory... The emergence of factories as a tool to facilitate testing is a code smell and this is a solution for that.

But are we solving the problem of factories with this solution? The current way Context is set up is that it's a centralised place for all dependencies that were previously being access with XXX.shared. However, we still pass all the other dependencies through constructors.

I wonder if the Context can hold the dependencies that we create in the constructors, too, which would simplify a lot for us.

Instead of:

final class MyClass {
 private let contentHasher: ContentHashing

  init(contentHasher: ContentHashing = ContentHasher()) {
   self.contentHasher = contentHasher
  }

  func hash(project: Project, context: Context) {
   contentHasher.hash(context.xcodeVersion())
   contentHasher.hash(project.name)
  }
}

It would be amazing if we could do something like:

struct MyClassContext {
  let xcodeVersion: XcodeVersion
  let contentHasher: ContentHashing
}

final class MyClass {
  func hash(project: Project, context: MyClassContext) {
   context.contentHasher.hash(context.xcodeVersion())
   context.contentHasher.hash(project.name)
  }
}

As currently, we basically have two ways how to do dependency injection. The swift-dependencies library from PointFree basically tries to do similar to above. Note the above is mostly a pseudocode but since adding context everywhere is a big effort, it's worth spending some extra time and make sure we do this right and explore all alternatives.

[pepicrft]

As currently, we basically have two ways how to do dependency injection. The swift-dependencies library from PointFree basically tries to do similar to above. Note the above is mostly a pseudocode but since adding context everywhere is a big effort, it's worth spending some extra time and make sure we do this right and explore all alternatives.

I started exploring this one today. I learned about what they use internally to pass state down through structured concurrency, TaskLocal, but I was unable to have tests running with a very basic example: ParallelTesting.zip.

I'm not opposed to adding dependencies, but if the implementation is small, like I believe could be the case here, I'm inclined to to that ourselves and be in control of the patterns. There were other problems/needs in the past, like Mockable, whose implementation would have been a huge cost for us.

[pepicrft]

An update here @danieleformichelli. We could talk to an Apple engineer who worked in Swift Concurrency about TaskLocal, which Swift Dependencies uses internally to propagate state down in structured concurrency. Still, we recommended not to use it because it was not designed for that and, therefore, might lead to issues, especially in the area of debugging, where a state (or dependency in this context) might not get propagated, leading to wasted time trying to understand Swift Dependencies or Structured Concurrency behaviors. I'd rather not throw us into that path. I'd refrain from exploring any other dependency-injection solution because Swift was not designed to make that ergonomic, so any solution will add unnecessary friction and potentially internal hacks that might break in the future.

There's still the decision of whether we should inject it in constructors or functions. The decision here will impact the ergonomics of most of our tests and the constructors of our utilities. If we embrace a world where we have control over isolating, we should aim to write fewer unit tests and more integration tests that bring more value. Performance shouldn't be an issue because we can parallelize and dog-food Tuist. In that world, where to let's say we have the following dependency graph that we are going to test A (class) -> B (struct) -> C (struct), and we'd like to write AIntegrationTests, if we inject the Context via the constructors, it's easy that B's constructor forgets to pass the context given by A when initializing C. So, I would like to move away from bloated constructors and factories that ended up being like that due to our constructor-based dependency injection. If we move from that, most of our structs and classes will have an init without arguments. If a given function needs a context, we declare it as an argument, and it's doubtful that we forget about it.

The TL;DR; of the plan that I'm proposing is:

• Less bloated constructors, factories to dependency-inject in tests, and fewer unit tests
• More context arguments in functions and acceptance tests.

I bet that the shift from constructors to functions will be net positive in ergonomics. Let me know what you think about it. If you are onboard, I'll go ahead, get the PR green, and continue with this work. We should further parallelize our codebase, but we are quite limited by our current approach.

[danieleformichelli]

Ciao 👋
Thanks for the exploration! 🙏

I have a doubt about this:

if we inject the Context via the constructors, it's easy that B's constructor forgets to pass the context given by A when initializing C. So, I would like to move away from bloated constructors and factories that ended up being like that due to our constructor-based dependency injection. If we move from that, most of our structs and classes will have an init without arguments. If a given function needs a context, we declare it as an argument, and it's doubtful that we forget about it.

As the context would be mandatory for the method, it could be mandatory for the constructor as well, why should you be more likely to forget to pass it?
I think exposing DI in the method is leaking a lot of internal details of the method, and I prefer to see the whole clalss as a "black box" taking some dependencies and exposing some methods, which also has the advantage of making the whole DI much less verbose (e.g. if I need a function of class X I also need to receive the dependencies of that function, while if everything is injected in the init, I just need to get injected that class, not its dependencies).

That said, if you are convinced that injecting through functions is better, I'll live with it :)

[kwridan]

Thanks for putting this together @pepicrft

I’m +1 for eliminating the singletons and their usage where possible so we can have a much clearer and deterministic dependencies within the Tuist codebase.

As the context would be mandatory for the method, it could be mandatory for the constructor as well, why should you be more likely to forget to pass it?
I think exposing DI in the method is leaking a lot of internal details of the method, and I prefer to see the whole clalss as a "black box" taking some dependencies and exposing some methods, which also has the advantage of making the whole DI much less verbose (e.g. if I need a function of class X I also need to receive the dependencies of that function, while if everything is injected in the init, I just need to get injected that class, not its dependencies).

Agree with @danieleformichelli on this 👍

The ergonomics of passing context to the functions isn’t ideal - take CachedManifestLoader for instance which internally has a lot of private methods, context needs to be daisy chained all the way through several other functions that don’t need it.

Moving more things to Context may not scale - if I understood the proposal, more dependencies would be added there? (e.g. FileHandler, System, etc… ?):

• Context would need to live in a module visible to all others (e.g. TuistSupport)
• Today it may hold common things used by most modules (e.g. FileHandling)
• Not all dependencies are needed by all modules (e.g. TuistGenerator doesn't need AsyncQueuer nor any networking related ones should they get introduced there too)
• For TuistKit and other higher level modules that have their own additional dependencies, they would be faced with 2 options
  • (1) Extend Context to contain the dependency the component needs (even if it's only specific to it) - meaning more things would need to move to TuistSupport
  • (2) Pass Context + the specific dependency to the component causing some inconsistency and confusion on how dependencies are dealt with

I find myself still leaning towards the solution @marciniwanicki proposed a few years ago #351 it's what scaled for us in our codebase. Perhaps a simpler form of it without any 3rd parties is what can work for Tuist. (i.e. keeping construct based DI, just revising that to be a bit more consistent / easier to deal with)

With #6105 being considered I’d be cautious of coupling TuistGenerator with any unnecessary dependencies.

[pepicrft]

I'm convinced. It'll rekindle @marciniwanicki's original proposal and build a miniature version over which we can move dependencies. I missed the point that we'd end up moving many dependencies down to TuistSupport to make them shareable through context, which is a bad idea. I didn't get this from @danieleformichelli's comment on "leaking internal details".
@kwridan, have you run into any hiccups with this approach, for example, in structure concurrency? I'm curious about the hiccups we might encounter if we follow this path.

With #6105 being considered, I’d be cautious about coupling TuistGenerator with any unnecessary dependencies.

Do you see as a sensible path having XcodeProjectGenerator (I'm intentionally dropping Tuist from the name to treat it as a generic project generation tool) depending on generic tools like FileHandler, which we could extract from Tuist?

[pepicrft]

I'm closing this one and I'll open a follow-up PR with a different approach inspired by @marciniwanicki's suggested approach.