-
Notifications
You must be signed in to change notification settings - Fork 45
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
feat: Move view cache to Blueprint hierarchy #531
base: main
Are you sure you want to change the base?
Changes from all commits
54f9c6d
da6e548
cd61a36
7750574
c7f0ad7
c76225d
07ffd85
36fff04
5a464a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
import Foundation | ||
|
||
extension Environment { | ||
private enum ViewCacheKey: EnvironmentKey { | ||
static let defaultValue: TypeKeyedCache? = nil | ||
} | ||
|
||
private static let fallback = TypeKeyedCache() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking we shouldn't have a static fallback. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah if we can get away with it. I haven't done an integration in POS yet, and I believe there is older code that just out of band measures elements with an empty environment, so we may need it for now. |
||
|
||
var inheritedViewCache: TypeKeyedCache? { | ||
self[ViewCacheKey.self] | ||
} | ||
|
||
public internal(set) var viewCache: TypeKeyedCache { | ||
get { | ||
if let inheritedViewCache { | ||
return inheritedViewCache | ||
} else { | ||
#if DEBUG | ||
do { | ||
/// Set a breakpoint on this `throw` if you'd like to understand where this error is occurring. | ||
/// | ||
/// We throw a caught error so that program execution can continue, and folks can opt | ||
/// in or out of stopping on the error. | ||
throw ViewCacheErrors.fallingBackToStaticCache | ||
} catch { | ||
|
||
/// **Warning**: Blueprint is falling back to a static `TypeKeyedCache`, | ||
/// which will result in prototype measurement values being retained for | ||
/// the lifetime of the application, which can result in memory leaks. | ||
/// | ||
/// If you are seeing this error, ensure you're passing the Blueprint `Environment` | ||
/// properly through your element hierarchies – you should almost _never_ be | ||
/// passing an `.empty` environment to methods, and instead passing an inherited | ||
/// environment which will be passed to you by callers or a parent view controller, | ||
/// screen, or element. | ||
/// | ||
/// To learn more, see https://github.com/square/Blueprint/tree/main/Documentation/TODO.md. | ||
|
||
} | ||
#endif | ||
|
||
return Self.fallback | ||
} | ||
} | ||
|
||
set { | ||
self[ViewCacheKey.self] = newValue | ||
} | ||
} | ||
|
||
private enum ViewCacheErrors: Error { | ||
case fallingBackToStaticCache | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import UIKit | ||
|
||
/// A cache that uses the value's type as the key. | ||
public final class TypeKeyedCache { | ||
|
||
private var views: [Key: AnyObject] = [:] | ||
|
||
// Intentionally internal. Not intended for public instantiation. | ||
init() {} | ||
|
||
// Returns the cached value for the Value type if it exists. If it doesn't | ||
// exist, it creates a new instance of Value, caches it, and returns it. | ||
// | ||
// - Parameter create: A closure that creates a new instance of Value. It | ||
// is only invoked when a cached value does not exist. The created value is | ||
// cached for future usage. | ||
public func value<Value: AnyObject>(_ create: () -> Value) -> Value { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, I went with the "access" pattern with an access closure so people aren't tempted to keep a reference to the cached measurement view, kind of like the swift pointer APIs where the pointer shouldn't be stored outside the access closure. |
||
let key = Key( | ||
elementType: ObjectIdentifier(Value.self) | ||
) | ||
|
||
if let existing = views[key] { | ||
return existing as! Value | ||
} else { | ||
let new = create() | ||
views[key] = new | ||
return new | ||
} | ||
} | ||
|
||
private struct Key: Hashable { | ||
let elementType: ObjectIdentifier | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,16 +128,22 @@ public struct AttributedLabel: Element, Hashable { | |
/// You can check if this value should be false via `NSAttributedString.needsNormalizingForView(...)` | ||
public var needsTextNormalization: Bool = true | ||
|
||
private static let prototypeLabel = LabelView() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @watt suggested we consider leaving this one as-is, at least for the initial iteration since its:
|
||
|
||
public var content: ElementContent { | ||
|
||
// We create this outside of the measurement block so it's called fewer times. | ||
let text = displayableAttributedText | ||
|
||
return ElementContent { constraint, environment -> CGSize in | ||
let label = Self.prototypeLabel | ||
label.update(model: self, text: text, environment: environment, isMeasuring: true) | ||
let label = environment.viewCache.value { | ||
LabelView() | ||
} | ||
|
||
label.update( | ||
model: self, | ||
text: text, | ||
environment: environment, | ||
isMeasuring: true | ||
) | ||
return label.sizeThatFits(constraint.maximum) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still a little nervous about cascading view caches through the environment. Won't it still leave open the possibility that nested blueprint view elements leak stuff through long(er)-lived caches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely possible, but largely the root blueprint view lives on the screen level.