Previews default to live implementations, but can we leverage unimplemented?

[dafurman]

Right now, tests spit out a handy error about a dependency being unmocked:

However, previews aren't equipped with the same capability, and they default to a live value.

I don't know that this is necessarily an "issue", but I wanted to open up some discussion around this. I feel like neither previews nor tests should default to live values, and that leaving them unspecified when they end up getting used should be considered a developer error, to ensure we're deliberate and thoughtful about dependencies that get used.

In the documentation here, there seems to be a similar sentiment:

Xcode previews are similar to tests in that you usually do not want to interact with the outside world, such as making network requests.

I don't know that I agree with the second sentence here though:

However, Xcode previews are dissimilar to tests in that it’s fine for dependencies to return some mock data. There’s no need to deal with “unimplemented” clients for proving which dependencies are actually used.

Just like with how you've described exhaustive dependencies in tests here, I really like the idea of being explicit about which dependencies are actually getting used in previews, so that devs aren't unknowingly depending on default mocks, or on live data that could change out from under them (or hit APIs and cost money! 😱)

However...I don't really know how this could be done today. XCTFail isn't an option for previews, and previews don't play nicely with fatalError as a way to indicate an error, so I don't think that approach would be great. These popups can be pretty annoying:

We could try doing something like creating runtime warnings, but I'm pretty sure if we're not fatalErroring, we'd still need a basic mock implementation to return in order to compile. Fortunately it looks like unimplemented() from XCTestDynamicOverlay does something similar to support placeholders already, so maybe it could just be a matter of updating those unimplemented() functions to error more nicely in previews?

Maybe we're simply stuck here without better error tooling in previews, but I'm curious to hear others' thoughts on this!

As a partial solution, it almost seems like we should delete this default value of previews defaulting to live. I'm curious to hear the motivation behind this.

[mbrandonw]

Hi @dafurman, thanks for bringing this up! It's a good question, and to be honest we don't quite remember why we chose what we did. We even discussed a few weeks ago that it seemed like it would have been better to have preview values take from the testValue by default.

I guess one thing that comes to mind is that it may seem surprising for previews to seem subtly broken if they take from the testValue. And that you will have to remember to override dependencies to get something on screen. But I don't know if that's all that bad of a problem, and certainly accessing live dependencies that could cost money or muddy server data is just as bad.

We'll discuss a bit more about changing the behavior, but please do share any other information you have on this or if anyone else wants to chime in!

[dafurman]

That's just not possible. We can only fatalError in that situation, which as you noted breaks the preview pretty badly.

Dang, but yeah I couldn't think of anything better here either.

And in fact, in the screenshot you provided, while the test does notify you that you didn't provide a testValue, it did actually use the liveValue under the hood. It had no choice but to do that. It just yelled loudly at you for doing it.

Ah, I didn't catch that! Yeah you're right I see that now here,

swift-dependencies/Sources/Dependencies/DependencyKey.swift

Line 241 in 33eead9

return Self.previewValue

testValue defaults to previewValue which defaults to live

So, what about doing this?

As a partial solution, it almost seems like we should delete this default value of previews defaulting to live.

No worries if you don't want to go this route, as obviously this wouldn't be a decision to take lightly because it'd be a breaking change to the library. Plus, it might be a little too opinionated and reduce convenience for devs.

Still, I'm uneasy with the idea of live values being used by default. At least in my company's codebase, I'm thinking we may want to explicitly make all dependency preview values default to barebones mocks, like this:

private enum AnalyticsKey: DependencyKey {
    static let liveValue: AnalyticsProtocol = Analytics()
    
    // 🟢 Provide a barebones, placeholder default for previews
    static let previewValue: AnalyticsProtocol = AnalyticsMock.placeholder
}

I'm mainly just wondering if it's worth this being enforced by the compiler to supply a previewValue manually. Anyways, I think I've got my solution at least!

[mbrandonw]

So, what about doing this?

As a partial solution, it almost seems like we should delete this default value of previews defaulting to live.

We are definitely willing to consider this option, and Stephen and I talked about it at length last week, however there's still something I'm not quite sure of. Even if we removed that default, it would not help you in your situation. The only time it would help is if you provide a testValue so that the previewValue can fallback to it.

And so then my question is: is it really that common of a situation that one is familiar enough with the library that they have reasons to provide a testValue implementation but not know that maybe they want to also provide a previewValue for certain dependencies? That doesn't seem to really be that common.

I think the more natural progression of using our library is that one will only provide a liveValue in the beginning because that's the easiest option. And then over time they will see the benefits of providing testValue and previewValue and start to do so.

[dafurman]

Even if we removed that defauhinkzlt, it would not help you in your situation. The only time it would help is if you provide a testValue so that the previewValue can fallback to it.

Ah that's because I realize I missed a spot. I think both of these two lines would need to be removed to achieve what I'm talking about, not just the first one:

1. swift-dependencies/Sources/Dependencies/DependencyKey.swift

   Line 169 in 33eead9

   public static var previewValue: Value { Self.liveValue }

2. swift-dependencies/Sources/Dependencies/DependencyKey.swift

   Line 251 in 33eead9

   public static var previewValue: Value { Self.testValue }

I tried this change out in a local copy of the library, and it gets the effect I'm looking for: The compiler now asks devs to specify their own previewValue, just as it does a liveValue:

I feel like this is ideal precisely for people who aren't familiar with the library, because this would lead to quick, natural discovery of previewValues, as it'd become a requirement.

I think the more natural progression of using our library is that one will only provide a liveValue in the beginning because that's the easiest option. And then over time they will see the benefits of providing testValue and previewValue and start to do so.

I think you're right about natural progression here though, and that people are probably gonna discover previewValues on their own one way or another. I'm just thinking this discovery could happen earlier in the library adoption, and that some potentially-confusing situations could be avoided if previews didn't default to live.

I'm planning on introducing the library into my team's codebase soon, so I'm just trying to anticipate pain points. I worry that my team will end up accidentally using live values to start with, without realizing that their previews are reliant on those. Fortunately, I think I can prevent this confusion among my team by just spreading awareness of previewValues and its default live behavior though, which is why I'm not super concerned about this. I'm mainly just wanting to have the discussion to hear opinions 😄

[dafurman]

Oh! I thought of a really good reason to not have previewValue become a required implementation by devs using the library: Some codebases may not be using previews at all.
Particularly, my suggestion wouldn't make sense for codebases that aren't UI-based - like CLI tools and such. Having previews be required would probably get pretty annoying for devs with these codebases. For that reason, I think the library's current behavior actually makes more sense than what I was originally suggesting, because it'd prevent the library from feeling too tailored towards UI-based codebases.

[jflan-dd]

+1 to having previewValue always default to testValue.

We generally think of dependencies as either "live" or "not live", and that mostly works for us in previews because many of our core dependencies split the testValue and liveValue implementations between modules, but there are a few dependencies that define the liveValue alongside the testValue and in those situations we have to remember to also define var previewValue: Self { testValue }.

I think the previewValue is a valuable customization point, but I wish it always defaulted to testValue. Explaining the subtle difference in behavior of preview dependencies based on which modules are linked into the preview target has been a small pain point when adopting the library on a large team.