Add documentation by dfed · Pull Request #4 · dfed/SafeDI

dfed · 2023-12-17T21:45:55Z

After a few weeks‘ of work, we have a working repository, and now: a readme!

codecov · 2023-12-17T21:53:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c2c6a40) 97.04% compared to head (e3c06b0) 97.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main       #4      +/-   ##
==========================================
- Coverage   97.04%   97.00%   -0.05%     
==========================================
  Files          36       36              
  Lines        7335     7335              
==========================================
- Hits         7118     7115       -3     
- Misses        217      220       +3

see 1 file with indirect coverage changes

kierajmumick

All looks pretty good! I have some comments and suggestions!

ahmdmhasn · 2023-12-19T21:19:15Z

+
+If a type is declared in third-party code, you can declare an extension the type in your code and decorate it with the `@ExternalInstantiable` macro to opt it into SafeDI.
+
+Let‘s walk through each of these macros in detail.


The inclusion of graphs enhances clarity regarding the various types. Without prior familiarity with the potency of dependency injection in the tree, comprehending this concept initially might prove challenging.

Also, I would suggest to mention the different Macros ahead and then jump into an example of each.

The intro is fine in this part

See the new cheat sheet section, and the updated "Using SafeDI" section. I think I've addressed what you're getting at. Let me know!

alright undid the cheat sheet sectioned put in a table at the top.

ahmdmhasn · 2023-12-19T21:47:30Z

+1. The type‘s SafeDI-injected properties are all `@Instantiated`
+2. The type is not instantiated by another `@Instantiable` type
+
+### Comparing SafeDI and Manual Injection: Key Differences


To highlight its strength, a useful method is to compare it with other Dependency Injection (DI) frameworks. Presenting the same key points in a table format, using X and ✔️, can effectively illustrate this comparison.

I have a Comparing SafeDI to other DI libraries section, but it's pretty light-weight. I like the idea of a table here, but I'm not sure we need to go that hard just yet. This section is more of an adoption guide.

cyrilchandelier · 2023-12-19T21:45:00Z

+
+#### Instantiating objects
+
+In a manual DI system, it is common to directly call your dependencies‘ `init(…)` methods. When utilizing SafeDI, you must rely on `@Instantiated`-decorated properties to instantiate your dependencies for you. Calling a dependency‘s `init(…)` method directly effectively exits the SafeDI-built dependency tree.


When utilizing SafeDI, you must rely on @Instantiated-decorated properties to instantiate your dependencies for you.

What happens if you don't? Say you start passing around a @Instantiated dependency your class received, will this cause any issue?

You can call init yourself, but then you're basically exiting the SafeDI dependency tree and there are no guarantees re when something is instantiated. Sounds like I should make that more clear here 🙂

If someone forgets to add @Instantiated to a property, is this something they will notice easily?

short answer: yes they will notice quickly and easily.

If they fail add @Instantiated, @Received, or @Forwarded to a property on an @Instantiatable type, then SafeDI will fail the build (with a FixIt) until they write an init method that does not include this property as an argument. If they accept the FixIt, they'll need to instantiate the property themselves in init(...), which should be a sign that they're not using DI for this property.

I've now answered Cyril's q in this section!

cyrilchandelier · 2023-12-19T21:54:00Z

+    public init(authService: AuthService, securePersistentStorage: SecurePersistentStorage) {
+        self.authService = authService
+        self.securePersistentStorage = securePersistentStorage
+    }


Any chance this initializer too can be generated?

In the simple case, it can! I've got a TODO in the codebase to simplify this.

When you have non-injected properties that must be set in init, though, you'll still need to write your own initializer. I like that today we can generate the structure of the initializer for you with a FixIt – if I started generating the initializer for you we'd lose the FixIt. I haven't figured out if the trade-off of simplifying the simple case while making the more complex (but still common!) case a bit more annoying is worth it yet.

Of course, now that I've typed that out... I could generate the FixIt only if you have a local property with an initializer... that'd be the best of both worlds. We’ll Get There™. Just gotta put in the time 🙂

ahmdmhasn · 2023-12-19T21:56:56Z

+
+The `ForwardingInstantiator` type is how SafeDI enables instantiating any `@Instantiable` type with a `@Forwarded` property. `ForwardingInstantiator` has two generics. The first generic must match the type of the `@Forwarded` property. The second generic matches the type of the to-be-instantiated instance.
+
+```swift


Can we add another simple example in UIKit for an App level injection as well?

We can, but honestly I have significantly less-strong opinions about the right way to define a root using SafeDI in UIKit-world.

Because AppDelegate and SceneDelegate must be separate objects, and neither object has a good way to reference the other (outside of the scene delegate referencing and casting UIApplication.shared.delegate)... there's no great way to make an application root like there is in SwiftUI.

In my own spikes I've been resorting to making a

@Instantiable public final class Root { public init(...) { ... } static let shared = Root() // uses initializer generated by SafeDI // Instantiated properties here ... }

And then utilizing Root.shared in both the AppDelegate and SceneDelegate. This is a pretty kludgy (but workable) approach, and is similar to how folk tend to share objects between these two types anyways. Alternatively, you could have separate dependency trees for your AppDelegate and SceneDelegate... but that seems like a recipe for pain since scenes and the app do need to share context.

So... yeah. I didn't want to have an opinion on this in the README, since there is no great option. Does that make sense?

I agree that having a UIKit example will aid folk who are adopting this system in UIKit, but also... it's not hard to make the leap from "how do I share dependencies between these two delegates" to "I should create a self-vending singleton".

kierajmumick · 2023-12-19T22:50:41Z

+@Instantiable(fulfillingAdditionalTypes: [UserService.self])
+public final class DefaultUserService: UserService {


I know we talked about this probably about 100 times, and I know you disagree due to differences in coding style across the industry, but seeing this in front of me really does make me want to say this again 😂

I really do think we should be able to infer that XYZProtocolName: ProtocolName fulfills the type ProtocolName by default. If fulfillingAdditionalTypes is provided, we then ignore the defualt.

Otherwise we're writing effectively boilerplate (We're writing the string UserService 3 times), which feels like something Macros should eliminate.

We disagree on the point of a macro! The point isn't eliminating boilerplate as much as making it easier to do a standard thing.

At a high level: DI systems are already quite magical, so I really want to avoid "we made this assumption, didn't have a good way to tell you that we did so, and gave you no way to opt out without changing your type name" scenarios.

Imagine we had:

public open class ViewController: UIViewController { ... } @Instantiable public final class ProfileViewController: ViewController { ... }

I kinda doubt we would want to make ProfileViewController also fulfill ViewController. And if ViewController were @Instantiable the decision tree gets even more complex. Of course, we could come up with some set of rules to handle these cases and document them here, but... this kind of magic would make the learning curve steeper for this library.

The learning curve for adopting automated DI is already kinda reasonably high. I do not want to make it more complicated when the alternative is "repeat yourself in a way that makes it very clear what you're doing".

kierajmumick · 2023-12-24T03:21:44Z

+
+/// A default implementation of `UserService` that can fulfill `@Instantiated`
+/// properties of type `UserService` or `DefaultUserService`.
+@Instantiable(fulfillingAdditionalTypes: [UserService.self])


This is fufilling additional types because DefaultUserService will be used to fulfill dependencies of type UserService and DefaultUserService. Is that right? Asking to make sure I understand clearly.

kierajmumick

Looks great to me!

dfed · 2023-12-24T23:45:55Z

Going to leave this PR open for a few more days to see if I can get another review pass with the new changes. Once this gets merged I'll release a 0.1.0 beta version of SafeDI!

NickEntin · 2023-12-24T10:03:47Z

+
+1. `public` or `open`
+
+2. Have a `public init(…)` or `open init(…)` that receives every injectable property


Can the initializers also receive non-injectable properties? (Is this an "at least" or "exactly")

They cannot. There must be one initializer that has arguments only for only injected (Instantiable, Forwarded, or Received) properties.

Under the hood, the reason for this is that this is the initializer that SafeDI uses to initialize your Instantiable object. And since SafeDI is only aware of (and has access to) properties in the DI tree... we wouldn't be able to make a call to an initializer with additional properties.

Going to leave this as-is for now since:

I'm struggling to come up with better wording here

There is a FixIt if you do this wrong

I need to take another pass here given 418bada – this initializer is now generated for you unless you have uninitialized properties on the type.

NickEntin · 2023-12-25T08:17:27Z

+
+#### Instantiating objects
+
+In a manual DI system, it is common to directly call your dependencies‘ `init(…)` functions. When utilizing SafeDI, you must rely on `@Instantiated`-decorated properties to instantiate your dependencies for you. Calling a dependency‘s `init(…)` function directly effectively exits the SafeDI-built dependency tree, which removes property lifecycle guarantees.


With the exception of the root, right?

Good call that I should make this explicit. Yeah, for the root of the dependency graph you must call init(). It's still true that you shouldn't call init(...) though... but yeah I should make this more clear.

Co-authored-by: Nick Entin <nckentn@gmail.com>

gonzalonunez · 2023-12-26T14:31:13Z

+✅ Compile-time safe
+


Aesthetic nit, I would maybe consider using regular markdown checkboxes here. If anything just to save you some space and get more content above-the-fold:

Compile-time safe

Thread safe

...

gonzalonunez · 2023-12-26T14:33:34Z

+
+✅ Clear error messages: never debug generated code
+
+## Using SafeDI


A single top-level code sample, maybe even just a condensed version of the example app's root view, would be really helpful to see before diving into the specifics behind each macro. It would sort of contextualize the details below.

Yeah I need to do another pass on this and pull a bunch of the below out into a wiki + focus the README on "how do I get started". That'll be a future round though.

gonzalonunez · 2023-12-26T14:35:40Z

+
+> Declare state objects as private to prevent setting them from a memberwise initializer, which can conflict with the storage management that SwiftUI provides
+
+`@Instantiated`, `@Forwarded`, or `@Received` objects may be decorated with [`@ObservedObject`](https://developer.apple.com/documentation/swiftui/ObservedObject). Note that `@Instantiated` objects declared on a `View` will be re-initialized when the view is re-initialized. You can find a deep dive on SwiftUI view lifecycles [here](https://www.donnywals.com/understanding-how-and-when-swiftui-decides-to-redraw-views/).


This feels like an important subtlety. Maybe a code-sample or a link to a Wiki in the repo that explains in detail?

💯. Will need to do this on a future pass, but yeah this is a tough one. The example app shows has an example of this in practice at least.

While it’s important and subtle, it is a subtlety inherent in SwiftUIs architecture, with or without this DI system.

It shouldn’t be the job of these docs to
the SwiftUI subtleties in detail. I do think the call out & link is likely enough!

dfed · 2023-12-27T04:05:09Z

I'm going to merge this, but I am still quite open to feedback on both this documentation and the API! I'll be pushing a beta 0.1.0 tag release of this repo shortly, but since we're still in beta it's a good time to provide API feedback!

One bit of feedback I've received offline from a couple folk is that much of the content in this README would be best split out into wiki docs, and the README should be refocused on more of a Quickstart guide (with clear code examples pulled from the sample project) + linking to the rest of the wiki. This is great feedback, and I agree with it, but in the spirit of shipping I'm going to merge this as-is and take a future action item to rework this.

dfed force-pushed the dfed--documentation branch 2 times, most recently from 418accb to d20b768 Compare December 17, 2023 21:51

dfed force-pushed the dfed--documentation branch 5 times, most recently from 052e8b9 to 5df5964 Compare December 19, 2023 05:51

dfed marked this pull request as ready for review December 19, 2023 05:58

dfed requested a review from martinmroz December 19, 2023 05:59

ahmdmhasn approved these changes Dec 19, 2023

View reviewed changes

dfed requested a review from ahmdmhasn December 19, 2023 07:52

kierajmumick reviewed Dec 19, 2023

View reviewed changes

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md