Content extraction is decoupled from `Inspectable` conformance #216

bachand · 2022-12-19T00:57:36Z

This PR is easiest to review commit by commit.

Changes

This PR follows my investigation in #213 which followed my question (#203) about the purpose of the Inspectable protocol.

With this PR, Inspectable is now a "marker" protocol with no requirements. Swift 5.7 introduces new functionality that enables us to call body on protocols like View that have an associated type. The lack of this functionality was a reason why it was necessary for views to conform to Inspectable in order to be inspected.

The public API of ViewInspector remains the same after this PR. My goal is to eliminate the need for views to conform to Inspectable in order to be inspected.

Testing

I am developing using Xcode 14.1 on macOS 13.1. My Mac has an M1 processor. When I open the Package.swift and run the tests (Cmd + u) for my Mac I see 4 failing tests on HEAD of the master branch.

Mac Tests - Before	Mac Tests - After

I cannot run the full test suite for iOS. I've filed an issue #217.

Reviewers

@nalexn

bachand · 2022-12-19T01:04:40Z

Sources/ViewInspector/ViewSearchIndex.swift

-            case .gesture:
+            case _ as any Gesture:
+                break
+            default:


We add a default case but we haven't made this code any less safe. Previously we would return nil if content.view did not conform to Inspectable.

bachand · 2022-12-19T01:08:29Z

Sources/ViewInspector/SwiftUI/CustomView.swift

@@ -2,15 +2,15 @@ import SwiftUI

 @available(iOS 13.0, macOS 10.15, tvOS 13.0, *)
 public protocol CustomViewType {
-    associatedtype T: Inspectable


Now that environment injection is decoupled from InspectableProtocol, we no longer need this requirement.

bachand · 2022-12-19T01:08:42Z

Sources/ViewInspector/SwiftUI/CustomView.swift

@@ -72,7 +72,7 @@ internal extension Content {
 @available(iOS 13.0, macOS 10.15, tvOS 13.0, *)
 public extension InspectableView where View: SingleViewContent {

-    func view<T>(_ type: T.Type) throws -> InspectableView<ViewType.View<T>> where T: Inspectable {


This was necessary because previously ViewType.View<T> needed to conform to InspectableProtocol though that requirement no longer exists.

bachand · 2022-12-19T01:57:53Z

Sources/ViewInspector/EnvironmentInjection.swift

-    var missingEnvironmentObjects: [String] {
+
+internal enum EnvironmentInjection {
+    static func missingEnvironmentObjects(for entity: Any) -> [String] {


I'm open to better names than entity.

bachand · 2022-12-19T02:10:03Z

Sources/ViewInspector/BaseTypes.swift

@@ -7,38 +7,6 @@ public protocol Inspectable {
    func extractContent(environmentObjects: [AnyObject]) throws -> Any
 }

-@available(iOS 13.0, macOS 10.15, tvOS 13.0, *)


In the Move code without changes commit I consolidate the extraction code into a single file.

bachand · 2022-12-19T02:14:18Z

Sources/ViewInspector/SwiftUI/CustomView.swift

@@ -55,14 +55,16 @@ internal extension Content {

    func extractCustomView() throws -> Content {
        let inspectable = try Inspector.cast(value: view, type: Inspectable.self)


It's possible to remove this line now and not cast to Inspectable.

Doing so will fail a couple of test.

In this PR we leave the requirement that views conform to Inspectable to be inspected.

bachand · 2022-12-19T02:17:48Z

Sources/ViewInspector/SwiftUI/CustomView.swift

@@ -126,21 +128,6 @@ public extension NSViewControllerRepresentable where Self: Inspectable {
    }
 }

-@available(iOS 13.0, macOS 10.15, tvOS 13.0, *)


These conformances are no longer necessary since we threw these errors in ContentExtractor.

ViewInspector/Sources/ViewInspector/ContentExtraction.swift

Lines 41 to 70 in 47d11e6

private func validateSource() throws {

switch contentSource.source {

#if os(macOS)

case is any NSViewRepresentable:

throw InspectionError.notSupported(

"Please use `.actualView().nsView()` for inspecting the contents of NSViewRepresentable")

case is any NSViewControllerRepresentable:

throw InspectionError.notSupported(

"Please use `.actualView().viewController()` for inspecting the contents of NSViewControllerRepresentable")

#endif

#if os(iOS) || os(tvOS)

case is any UIViewRepresentable:

throw InspectionError.notSupported(

"Please use `.actualView().uiView()` for inspecting the contents of UIViewRepresentable")

case is any UIViewControllerRepresentable:

throw InspectionError.notSupported(

"Please use `.actualView().viewController()` for inspecting the contents of UIViewControllerRepresentable")

#endif

#if os(watchOS)

case is any WKInterfaceObjectRepresentable:

throw InspectionError.notSupported(

"""

Please use `.actualView().interfaceObject()` for inspecting \

the contents of WKInterfaceObjectRepresentable

""")

#endif

default:

return

}

}

bachand · 2022-12-19T02:19:50Z

Package.swift

@@ -1,4 +1,4 @@
-// swift-tools-version:5.3
+// swift-tools-version:5.7


We need to target Swift 5.7 to be able to access this functionality: https://github.com/apple/swift-evolution/blob/main/proposals/0309-unlock-existential-types-for-all-protocols.md

bachand · 2022-12-19T02:30:30Z

@nalexn I'm curious to see what you think of this PR. I'm very open to making revisions (e.g. adding tests, updating naming, restructuring). Lmk!

nalexn · 2022-12-20T05:03:42Z

That's an interesting take, @bachand I'll try to find some time this week to review & complete your solution!

nalexn · 2022-12-22T14:27:44Z

I've continued the process of de-Inspectable-fication in this branch. Haven't reviewed the original code in the PR yet, just continued building on top of it. There is no doubt this PR will be a huge milestone for the library, yet, there is quite some work left to make Inspectable conformance a true legacy that can easily be removed. Too many APIs have this restriction, and when I remove the restriction, they start to collide with more loose APIs. I'll continue exploration

bachand · 2022-12-23T02:49:27Z

I've continued the process of de-Inspectable-fication in this branch.

I checked out the branch. I see you added a protocol called SwiftUICitizen, and that you conformed SwiftUI library views to this protocol. I'm curious to learn more about why you added this protocol and its purpose internal to the library (so that I can follow along!).

Haven't reviewed the original code in the PR yet, just continued building on top of it.

👍

There is no doubt this PR will be a huge milestone for the library, yet, there is quite some work left to make Inspectable conformance a true legacy that can easily be removed.

Agreed. I felt this as well as I tried to push further when I was working on this PR.

Too many APIs have this restriction, and when I remove the restriction, they start to collide with more loose APIs. I'll continue exploration

Makes sense. Keep me in the loop about how I can be helpful!

nalexn · 2022-12-23T07:42:20Z

I'm curious to learn more about why you added this protocol and its purpose internal to the library (so that I can follow along!).

The library was using Inspectable not only for cracking the custom views but also for distinguishing them from the standard SwiftUI views. As I started removing Inspectable from APIs things started falling apart, for example, find functions would always appeal to ContentExtractor because it'd now think all Views are custom.

Since we've removed a distinctive mark from custom views, this now had to be added to standard views, thus SwiftUICitizen mark. I was also hoping to use this for maintaining the inspect APIs, which have different overloads for custom vs standard views, and I got this working with one exception - extension ModifiedContent: SwiftUICitizen. For some reason compiler continues to not notice the conformance when selecting the inspect overload among a few, and this drives me nuts. I tend to think we'll need to make breaking API changes for this version and remove / deprecate a few inspect overloads. This may be the best timing to do this actually, as there are requests to support await inspection, which would mean the introduction of even more overloads.
––
Update: I managed to avoid changing the public API while removing Inspectable requirement from bunch of public functions. So far it goes smooth. We still need to do the same for ViewModifier's and Gesture's API.
In the end, if we find a way to go without SwiftUICitizen conformance, we should def remove it. I consider it a mid term quick solution.

nalexn · 2022-12-24T09:52:17Z

I've completed Inspectable protocol deprecation. This is now part of 0.9.3 release branch. @bachand thanks for this PR!

bachand · 2023-01-03T18:22:38Z

Thanks @nalexn for finishing the work and for publishing the 0.9.3 release 🙏

bachand added 2 commits December 18, 2022 16:41

Remove only usage of entity property

c30015a

Remove Content.InspectableEntity

91b9aa6

bachand changed the title ~~Mb inspectable is marker protocol~~ Content extraction is decoupled from Inspectable conformance Dec 19, 2022

bachand mentioned this pull request Dec 19, 2022

[Do not merge] Exploration into removing Inspectable conformance requirement #213

Closed

bachand commented Dec 19, 2022

View reviewed changes

bachand force-pushed the mb--inspectable-is-marker-protocol branch from 8d32d63 to 4bb9424 Compare December 19, 2022 01:06

bachand commented Dec 19, 2022

View reviewed changes

Update environment injection code to use static functions

cc22be8

bachand force-pushed the mb--inspectable-is-marker-protocol branch 6 times, most recently from af04454 to 961e0a6 Compare December 19, 2022 01:39

bachand commented Dec 19, 2022

View reviewed changes

bachand force-pushed the mb--inspectable-is-marker-protocol branch from 961e0a6 to 967bf30 Compare December 19, 2022 02:01

Remove Inspectable requirement from CustomView and CustomViewModifier

1b6504d

bachand force-pushed the mb--inspectable-is-marker-protocol branch from 967bf30 to f5ddd47 Compare December 19, 2022 02:02

bachand added 7 commits December 18, 2022 18:04

Remove Inspectable requirement from Gesture

f163a24

Move code without changes

e2a32aa

ContentExtraction.swift

2954039

Extraction extensions are independent of InspectableProtocol

5df23a0

Introduce ContentExtractor

47d11e6

Use ContentExtractor instead of Inspectable to extract content

04a1979

Increase minimum Swift version

6b81693

bachand force-pushed the mb--inspectable-is-marker-protocol branch from f5ddd47 to 6b81693 Compare December 19, 2022 02:04

bachand commented Dec 19, 2022

View reviewed changes

Increase Swift version in podspec

6e4fdc9

bachand marked this pull request as ready for review December 19, 2022 02:25

bachand mentioned this pull request Dec 19, 2022

Question: why do views need to conform to Inspectable to be inspected? #203

Closed

nalexn added the pending release A fixed issue that'll be released in an upcoming update label Dec 24, 2022

nalexn merged commit b65d0bd into nalexn:master Dec 25, 2022

nalexn removed the pending release A fixed issue that'll be released in an upcoming update label Dec 25, 2022

bachand deleted the mb--inspectable-is-marker-protocol branch January 3, 2023 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Content extraction is decoupled from `Inspectable` conformance #216

Content extraction is decoupled from `Inspectable` conformance #216

bachand commented Dec 19, 2022 •

edited

Loading

bachand Dec 19, 2022 •

edited

Loading

bachand Dec 19, 2022

bachand Dec 19, 2022

bachand Dec 19, 2022

bachand Dec 19, 2022

bachand Dec 19, 2022 •

edited

Loading

bachand Dec 19, 2022

bachand Dec 19, 2022

bachand commented Dec 19, 2022

nalexn commented Dec 20, 2022

nalexn commented Dec 22, 2022 •

edited

Loading

bachand commented Dec 23, 2022 •

edited

Loading

nalexn commented Dec 23, 2022 •

edited

Loading

nalexn commented Dec 24, 2022

bachand commented Jan 3, 2023

		@@ -55,14 +55,16 @@ internal extension Content {

		func extractCustomView() throws -> Content {
		let inspectable = try Inspector.cast(value: view, type: Inspectable.self)

	private func validateSource() throws {
	switch contentSource.source {
	#if os(macOS)
	case is any NSViewRepresentable:
	throw InspectionError.notSupported(
	"Please use `.actualView().nsView()` for inspecting the contents of NSViewRepresentable")
	case is any NSViewControllerRepresentable:
	throw InspectionError.notSupported(
	"Please use `.actualView().viewController()` for inspecting the contents of NSViewControllerRepresentable")
	#endif
	#if os(iOS) \|\| os(tvOS)
	case is any UIViewRepresentable:
	throw InspectionError.notSupported(
	"Please use `.actualView().uiView()` for inspecting the contents of UIViewRepresentable")
	case is any UIViewControllerRepresentable:
	throw InspectionError.notSupported(
	"Please use `.actualView().viewController()` for inspecting the contents of UIViewControllerRepresentable")
	#endif
	#if os(watchOS)
	case is any WKInterfaceObjectRepresentable:
	throw InspectionError.notSupported(
	"""
	Please use `.actualView().interfaceObject()` for inspecting \
	the contents of WKInterfaceObjectRepresentable
	""")
	#endif
	default:
	return
	}
	}

		@@ -1,4 +1,4 @@
		// swift-tools-version:5.3
		// swift-tools-version:5.7

Content extraction is decoupled from Inspectable conformance #216

Content extraction is decoupled from Inspectable conformance #216

Conversation

bachand commented Dec 19, 2022 • edited Loading

Changes

Testing

Reviewers

bachand Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

bachand Dec 19, 2022

Choose a reason for hiding this comment

bachand Dec 19, 2022

Choose a reason for hiding this comment

bachand Dec 19, 2022

Choose a reason for hiding this comment

bachand Dec 19, 2022

Choose a reason for hiding this comment

bachand Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

bachand Dec 19, 2022

Choose a reason for hiding this comment

bachand Dec 19, 2022

Choose a reason for hiding this comment

bachand commented Dec 19, 2022

nalexn commented Dec 20, 2022

nalexn commented Dec 22, 2022 • edited Loading

bachand commented Dec 23, 2022 • edited Loading

nalexn commented Dec 23, 2022 • edited Loading

nalexn commented Dec 24, 2022

bachand commented Jan 3, 2023

Content extraction is decoupled from `Inspectable` conformance #216

Content extraction is decoupled from `Inspectable` conformance #216

bachand commented Dec 19, 2022 •

edited

Loading

bachand Dec 19, 2022 •

edited

Loading

bachand Dec 19, 2022 •

edited

Loading

nalexn commented Dec 22, 2022 •

edited

Loading

bachand commented Dec 23, 2022 •

edited

Loading

nalexn commented Dec 23, 2022 •

edited

Loading