add LocalStorageKey; rework LocalStorage API #30

lukaskollmer · 2025-01-28T13:51:38Z

add LocalStorageKey; rework LocalStorage API

♻️ Current situation & Problem

LocalStorage currently identifies entries in the storage via either optional, raw untyped strings, or via the static type of the to-be-stored/loaded value.
While this approach does lead to a nice-looking API, it is problematic for several reasons:

identifying stored entries using string literals makes it easy to misspel something, or to accidentally use the wrong key
the fact that there's no static coupling between a key and the type it's used to store/load, means that there's nothing that would prevent someone from acidentally decoding a value into an incorrect type (i.e., a different type than what was encoded into the LocalStorage entry)
the fact that parameters such as e.g. the encoder/decoder or the storage setting, both of which determine how a value is stored/loaded, need to be explicitly specified at both calls to store() as well as calls to read(), means that it is easy to mismatch these parameters
- eg: if you use a PropertyListEncoder when storing some value into the LocalStorage, you need to make sure that you also use the matching PropertyListDecoder when reading the stored value.
  - since the current API defaults both the encoder in store() and the decoder in read() to the JSONEncoder (and `JSONDecoder, respectively), makes it too easy to miss the parameter somewhere
- the same applies to the storageSetting parameter: all matching calls to store() and read() that operate on the same storage key must specify the same storage setting.
  - this is currently not enforced, and with the current API there is no way of enforcing this
the fact that storage keys are optional (if the key is nil (the default), the unqualified typename of the to-be-stored/loaded type will be used instead) is also likely to lead to unexpected issues:
- since the unqualified typename doesn't take nesting / access control into account, it can easily happen that two calls to store()/read() that operate on completely different types (potentially even in completely different modules!) end up operating on the same LocalStorage entry, because the unqualified typename will be the same in both cases

This PR attempts to address these issues, by restructuring and redesigning the LocalStorage API:

Entries in the storage are no longer identified by raw strings which are passed to the store()/load() calls
Entries are no longer identified by the typename of the value being stored/loaded
We instead have a LocalStorageKey<Value> type, which are intended to be long-lived objects that are used to access the LocalStorage
- the LocalStorageKey encapsulates all information required to store data into the storage / load data from it
- since every store/load for an entry will use the same LocalStorageKey instance, we can ensure that every store/load for the same entry will use matching decoders/encoders, and will use the same storageSetting.
- it also improves the typing, by associating every storage key with its underlying stored type
- in order to prevent race conditions and other potential concurrency issues, the LocalStorageKey also acts as a read-write lock when accessing the storage entry identified by it
In addition to these changes, the LocalStorageKey-based API also makes it easier to support storing additional currently not supported types, such as e.g. NSSecureCoding-compliant types
Note this PR removes support for CodableWithConfiguration, which previously was supported but (AFAICT) not used.
Note: SpeziSecureStorage is currently untouched; it might make sense to have something similar there as well, but it'd of course require a separate, dedicated key type and would be out-of-scope for this PR.
This PR also adds a small convenience property wrapper, @LocalStorageEntry, which makes it easier to use the LocalStorage API from within a SwiftUI View, and is e.g. able to observe a key's value and update the view in response.

⚙️ Release Notes

Completely reworked the LocalStorage API:
- Values are stored/loaded via LocalStorageKeys, which define an entry in the storage.
- LocalStorageKeys need to explicitly specify their underlying raw keys (a String), and are encouraged to use reverse-DNS-notation in order to avoid collisions

📚 Documentation

The documentation has been adapted to the new API.

✅ Testing

The tests have been adapted to the new API.

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

I agree to follow the Code of Conduct and Contributing Guidelines.

codecov · 2025-01-28T14:20:35Z

Codecov Report

Attention: Patch coverage is 90.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (935a7e1) to head (6a9522e).

Files with missing lines	Patch %	Lines
Sources/SpeziLocalStorage/LocalStorageKey.swift	75.00%	10 Missing ⚠️
Sources/SpeziLocalStorage/LocalStorageEntry.swift	93.11%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #30      +/-   ##
==========================================
+ Coverage   88.75%   90.04%   +1.29%     
==========================================
  Files           6        8       +2     
  Lines         471      552      +81     
==========================================
+ Hits          418      497      +79     
- Misses         53       55       +2

Files with missing lines	Coverage Δ
Sources/SpeziLocalStorage/LocalStorage.swift	`92.26% <100.00%> (+8.41%)`	⬆️
...ources/SpeziLocalStorage/LocalStorageSetting.swift	`97.30% <100.00%> (ø)`
Sources/SpeziLocalStorage/LocalStorageEntry.swift	`93.11% <93.11%> (ø)`
Sources/SpeziLocalStorage/LocalStorageKey.swift	`75.00% <75.00%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 935a7e1...6a9522e. Read the comment docs.

(the second part is still work in progress)

lukaskollmer · 2025-01-28T19:42:03Z

Sources/SpeziLocalStorage/LocalStorageEntry.swift

i'm not overly experienced when it comes to using Combine, so please let me know whether what i'm doing here makes sense / is the correct approach

Looks alright to me, even tho I'm also rather inexperienced with Combine.

philippzagar

Thanks for the great PR @lukaskollmer! 🚀
Mostly had some smaller nits and long-shot ideas. What's important is to update the rest of the documentation.
And yes, a similar PR in the SecureStorage would be great as well!

philippzagar · 2025-01-29T17:46:39Z

Tests/UITests/TestApp/LocalStorageTests/LocalStorageLiveUpdateTestView.swift

+}
+
+
+struct LocalStorageLiveUpdateTestView: View { // swiftlint:disable:this file_types_order


Cool test, thanks!

philippzagar · 2025-01-29T17:47:37Z

Tests/UITests/TestAppUITests/LocalStorageTests.swift

+        let numbers = (0..<17).map { _ in Int.random(in: 0..<5) }
+        for number in numbers {
+            app.buttons["\(number)"].tap()
+            XCTAssertTrue(app.staticTexts.matching(NSPredicate(format: "label MATCHES %@", "number.*\(number)")).element.waitForExistence(timeout: 0.5))


The timeouts might be a bit low for the CI (we usually have around 2 sec), but we can evolve that if needed in the future.

i added the small delay since not having any delay would in some cases not catch the update (afaik the update happens on the next run loop invocation, so there is a small delay we need to account for)

philippzagar · 2025-01-29T17:50:43Z

Sources/SpeziLocalStorage/SpeziLocalStorage.docc/SpeziLocalStorage.md

+### Defining Storage Keys
+
+`LocalStorage` uses unique ``LocalStorageKey``s to .
+
+You define storage keys by placing a static non-computed properties of type ``LocalStorageKey`` into an extension on the ``LocalStorageKeys`` type:
+
+```swift
+extension LocalStorageKeys {
+    static let note = LocalStorageKey
+}
+```
+
+
+### Storing and Loading Data


Thanks for the updated docs! 🚀 I feel there are quite some other doc parts that we need to updated in the README and DocC

good catch; i'll look over the docs and make sure everything is in sync and up to date

philippzagar · 2025-01-29T17:51:42Z

Sources/SpeziLocalStorage/LocalStorageKey.swift

+        encode: @Sendable @escaping (Value) throws -> Data,
+        decode: @Sendable @escaping (Data) throws -> Value?


Cool idea, I like it!

philippzagar · 2025-01-29T17:54:09Z

Sources/SpeziLocalStorage/LocalStorageKey.swift

+    let setting: LocalStorageSetting
+    let encode: @Sendable (Value) throws -> Data
+    let decode: @Sendable (Data) throws -> Value?
+    private let lock = RWLock()


Just as a long shot: Should we aim to move away from manual locking and go towards an async/await actor-based implementation here? This would of course then also propagate to the public layer, which probably is not wanted. Might be overkill, just putting it out there.

that was @PSchmiedmayer's idea as well, but there's, from what i can tell, some issues with that approach:

with actors, we wouldn't be able to allow concurrent reads while enforcing exclusive writes (which currently is allowed).
whether this is something we actually need is a different question of course (the answer is almost definitely no, but i'd argue it does feel more elegant this way)

were we to use actors, we'd need to make all store() / load() calls async (that'd be the entire point, of course), which might be overkill, since the likelihood of parallel reads is already pretty low (plus, with the current implementation parallel reads would be fine anyway), and parallel writes are already synchronized currently.

more importantly, we wouldn't be able to have a property-wrapper for accessing storage entries anymore, since the wrappedValue can't be async.

Yep I share these concerns as well, I think the downsides of actor isolation outway the benefits in this case, so from my side I'm fine with keeping the RWLock, even tho it doesn't feel super swifty.

philippzagar · 2025-01-29T18:24:42Z

Sources/SpeziLocalStorage/LocalStorageEntry.swift

+
+
+/// Access ``LocalStorage`` entries within a SwiftUI View.
+@propertyWrapper


Great idea to introduce that! 🚀
Would be great to get a small usage doc in here for the wrapper as well as writing down the need for the LocalStorage module in the env.

@lukaskollmer Might make sense to define the wrapper like this, just on a View?
https://github.com/StanfordSpezi/SpeziLLM/blob/26b1e07756556b1b329f3a9a3267a5f99c0c2e78/Sources/SpeziLLMOpenAI/FunctionCalling/LLMFunctionParameterWrapper.swift#L83

not sure what you mean / how that would would work. @Property is a property wrapper which gets its value externally injected (as part of handling the LLM function call), which isn't something we can do with SwiftUI (if that's what you're talking about).

or are you referring to some other aspect of @Parameter?

philippzagar · 2025-01-29T18:25:29Z

Sources/SpeziLocalStorage/LocalStorage.swift

+    ///     If the closure returns `nil`, the entry will be removed from the `LocalStorage`.
+    ///
+    /// - throws: if `transform` throws,
+    public func modify<Value>(_ key: LocalStorageKey<Value>, transform: (Value?) throws -> Value?) throws {


Nit: Would be great to test this function

philippzagar · 2025-01-29T18:26:33Z

Sources/SpeziLocalStorage/LocalStorage.swift

+            if let value {
+                try storeImp(value, for: key)
+            } else {
+                try delete(key)


Nit: Would be great to test this branch

philippzagar · 2025-01-29T18:27:29Z

Sources/SpeziLocalStorage/LocalStorageEntry.swift

Looks alright to me, even tho I'm also rather inexperienced with Combine.

philippzagar · 2025-01-29T18:38:30Z

Tests/UITests/TestApp/LocalStorageTests/LocalStorageLiveUpdateTestView.swift

+    @LocalStorageEntry(.number) private var number
+
+    var body: some View {
+        LabeledContent("number", value: number.map(String.init) ?? "–")


Maybe test a small write to the property wrapper as well?

who would've thought testing the code would reveal the bugs?

lukaskollmer added 3 commits January 28, 2025 14:24

add LocalStorageKey; rework LocalStorage API

e5d0505

make it pretty

8886d5e

hmmm

6e8e6dd

lukaskollmer added 5 commits January 28, 2025 16:18

add @LocalStorageEntry property wrapper

6ca9e97

add tests for @LocalStorageEntry

17f9e47

make swiftlint happy, learn how combine works

1740b18

(the second part is still work in progress)

mmmmm

2929d93

some > any

e416c07

lukaskollmer requested review from PSchmiedmayer and philippzagar January 28, 2025 19:40

lukaskollmer commented Jan 28, 2025

View reviewed changes

lukaskollmer marked this pull request as ready for review January 29, 2025 06:50

philippzagar approved these changes Jan 29, 2025

View reviewed changes

lukaskollmer added 4 commits January 29, 2025 20:40

also test @LocalStorageEntry setter

b8ee13b

change modify signature, add a test case

85f49fc

swiftlint

1cd0095

extend test coverage, fix a bug along the way

6a9522e

who would've thought testing the code would reveal the bugs?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add LocalStorageKey; rework LocalStorage API #30

add LocalStorageKey; rework LocalStorage API #30

lukaskollmer commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading

lukaskollmer Jan 28, 2025

philippzagar Jan 29, 2025

philippzagar left a comment

philippzagar Jan 29, 2025

philippzagar Jan 29, 2025

lukaskollmer Jan 29, 2025

philippzagar Jan 29, 2025

lukaskollmer Jan 29, 2025

philippzagar Jan 29, 2025

philippzagar Jan 29, 2025

lukaskollmer Jan 29, 2025

philippzagar Jan 30, 2025

philippzagar Jan 29, 2025

philippzagar Jan 30, 2025

lukaskollmer Jan 30, 2025

philippzagar Jan 29, 2025

philippzagar Jan 29, 2025

philippzagar Jan 29, 2025

philippzagar Jan 29, 2025

		}


		struct LocalStorageLiveUpdateTestView: View { // swiftlint:disable:this file_types_order

		encode: @Sendable @escaping (Value) throws -> Data,
		decode: @Sendable @escaping (Data) throws -> Value?



		/// Access ``LocalStorage`` entries within a SwiftUI View.
		@propertyWrapper

add LocalStorageKey; rework LocalStorage API #30

Are you sure you want to change the base?

add LocalStorageKey; rework LocalStorage API #30

Conversation

lukaskollmer commented Jan 28, 2025 • edited Loading

add LocalStorageKey; rework LocalStorage API

♻️ Current situation & Problem

⚙️ Release Notes

📚 Documentation

✅ Testing

📝 Code of Conduct & Contributing Guidelines

codecov bot commented Jan 28, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippzagar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaskollmer commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading