Don’t assume CastableLookup #3011
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If the observed value in `UserDefaults` is a `RawRepresentable` type, simply casting to `Value` might fail because the raw representing type could be different from `Value`. Instead of querying `UserDefaults` directly here, we should call `loadValue()` which will do the right thing for raw representable types (esp. try to call `Value.init(rawValue:)` with the value obtained from `UserDefaults`).
stephencelis
approved these changes
Apr 25, 2024
|
Thanks for the review, I’ll add a commit for the test. |
|
@stephencelis added the test. This illustrates nicely how the type stored in user defaults is a |
|
Ah that test failure was fixed on another PR. Will merge this now, thanks again! |
stephencelis
merged commit 03d4037
into
pointfreeco:shared-state-beta
5 of 7 checks passed
mbrandonw
added a commit
that referenced
this pull request
Apr 29, 2024
* Shared State * fix compiler errors in 5.7 * fixes and tests * fix * fix * wip * longer sleeps * fix test * fix tests * Clean up some typos in SharingState.md (#2860) * public inits * wip * wip * add test for shared state and onChange * Case study for sandboxing shared state * tweaks to case study * wip * add tests to sandboxing case study * rename files * wip * simplify sync up tests * wip * fix * wip * fix $shared.publisher non-determinism and write test that currently fails but ideally would not. * more docs and a test for autoclosure * more docs * wip * update todo with persistence * wip * wip * support default `nil` optionals in `@Shared` * Introduce unavailable overload for better diagnostics * Fix autocomplete from `@Shared(.` * Revert "Fix autocomplete from `@Shared(.`" This reverts commit fd1798f. * Fix defaults * wip * Give persistence keys a synchronous update interface (#2880) * Don't use async sequence for persistence * wip * wip * wip * Update Sources/ComposableArchitecture/SharedState/PersistenceKey.swift Co-authored-by: Hal Lee <hal@lee.me> --------- Co-authored-by: Hal Lee <hal@lee.me> * Revert sharing in Todos for now * wip * wip * Pass initial shared value to strategy / register app storage (#2904) * Pass initial shared value to strategy / register app storage This PR modifies the `PersistenceKey` protocol so that its `load` and `subscribe` endpoints are handed the initial value and must return a non-optional value. By feeding this value in, we can ensure that `.appStorage` declared in a feature is registered outside the feature. * wip * wip * wip * wip * wip * wip * swift-format * Update Sources/ComposableArchitecture/Documentation.docc/Articles/SharingState.md Co-authored-by: Pyry Jahkola <pyry.jahkola@iki.fi> * non-exhaustive fix * wip * remove decodable for now * Fix app storage registration with `nil` values * fix * wip * tutorial * fixes * lots of tutorial fixes * more tutorial fixes * more tutorial fixes * more tutorial fixes * tutorial fixes * wip * re-arrange test * Remove store shared preview quarantine * Add support for \.defaultInMemoryStorage (#2965) * fix * Added Privacy Manifest file (#2930) * Added Privacy Manifest file * Update Package@swift-5.9.swift --------- Co-authored-by: Stephen Celis <stephen.celis@gmail.com> * remove warning overloads * write default instead of register this is more consistent with SwiftUI's property wrapper, and causes less strangeness when multiple repeat properties have different defaults * move privacy manifest * wip * wip * EphemeralFileStorage -> InMemoryFileStorage * documented gotcha * add test * Shared state beta task snaps (#2976) * wip * wip * wip * wip * wip * wip * wip --------- Co-authored-by: Brandon Williams <mbrandonw@hey.com> * wip * Use notification center instead of KVO for user defaults observation. (#2978) * Use notification center instead of KVO for user defaults observation. * wip * wip * Introduce @SharedReader (#2979) * wip * wip * wip * wip * wip * wip * Introduce @SharedReader. * wip * wip * sendable * wip * wip --------- Co-authored-by: Stephen Celis <stephen@stephencelis.com> * Make Shared.Subscription.cancel public (#2983) * added failing test * Add convenience initializers * Save to file storage when app is about to be terminated (#2992) * Save file storage on termination too. * wip * Shared change tracking enhancements (#2989) * wip * wip * wip * wip * Add unavailable conformance * wip * wip * wip * Don't need to check for previews. * Add default providing persistence key (#2980) * wip - default providing key * warn on default access * Revert "warn on default access" This reverts commit 3870645. * wip * wip * wip - tests * wip - reader key * fix docs * A few changes. * A few more changes. * docs --------- Co-authored-by: Brandon Williams <mbrandonw@hey.com> * Fix optional shared defaults. * undo last commit but keep test * wip * Better shared state change tracking and `TestStore` interactions (#2995) * A fix for shared assertion. * fixes * wip --------- Co-authored-by: Brandon Williams <mbrandonw@hey.com> * docs * docs * wip * Throttle for 1 second. * fix typo in examples: `let` -> `var` (#2999) Co-authored-by: Andreas Tielmann <atielmann@deloitte.de> * wip * wip * fix observing projection * Ping `Shared.publisher` in `willSet` so prev/next values observable * wip * wip * remove sandbox demo * wip * wip * recover inlining * wip * wip * fix docc * wip * wip * re-entrant test * wip * Don’t assume CastableLookup (#3011) * Don’t assume CastableLookup If the observed value in `UserDefaults` is a `RawRepresentable` type, simply casting to `Value` might fail because the raw representing type could be different from `Value`. Instead of querying `UserDefaults` directly here, we should call `loadValue()` which will do the right thing for raw representable types (esp. try to call `Value.init(rawValue:)` with the value obtained from `UserDefaults`). * Add test --------- Co-authored-by: Stephen Celis <stephen@stephencelis.com> * Make `FileStorage` more opaque (#3010) * Make `FileStorage` more opaque Previously, we had a public protocol and conformances, but we don't expect anyone to conform outside the library, so instead let's hide things in a more opaque struct. The downside of this PR is that it's a breaking API change pretty late in the game: ```diff -$0.defaultFileStorage = InMemoryFileStorage() +$0.defaultFileStorage = .inMemory ``` We could add public deprecated functions that emulate the old initializers if we think it's worth it... * wip * wip * wip * wip * wip * wip * fix for 5.7.1 * sync ups clean up * more docs * more tests * shared testing tiups * wip * wip * Fix typo. (#3014) * more shared state docs * relax version of swift-dependencies. * dep change * fix == and hash on Shared and more tests * revert == changes * add test * Remove hashable conformance from shared * remove conditional encodability from shared * wip * remove syncups tutorial * wip * fix * fix file storage deletion resubscription * wip * wip * wip * wip * wip * fix * eager throttle --------- Co-authored-by: Brandon Williams <mbrandonw@hey.com> Co-authored-by: NF <4764329+NFulkerson@users.noreply.github.com> Co-authored-by: Hal Lee <hal@lee.me> Co-authored-by: Brandon Williams <135203+mbrandonw@users.noreply.github.com> Co-authored-by: Pyry Jahkola <pyry.jahkola@iki.fi> Co-authored-by: Daniel Lyons <72824209+DandyLyons@users.noreply.github.com> Co-authored-by: Hilton Campbell <github@crosswaterbridge.com> Co-authored-by: Luke Redpath <lredpath@community.com> Co-authored-by: andtie <andreas.tielmann@gmail.com> Co-authored-by: Andreas Tielmann <atielmann@deloitte.de> Co-authored-by: Alex Kovács <alex@kobachi.jp> Co-authored-by: Zev Eisenberg <zev@zeveisenberg.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the observed value in
UserDefaultsis aRawRepresentabletype, simply casting toValuemight fail because the raw representing type could be different fromValue. Instead of queryingUserDefaultsdirectly here, we should callloadValue()which will do the right thing for raw representable types (esp. try to callValue.init(rawValue:)with the value obtained fromUserDefaults).Or am I missing something and there is a reason this is using
UserDefaults.value(forKey:)directly? I am especially curious becauseCastableLookupon line 374 is usingUserDefaults.object(forKey:)instead, so it does look somewhat deliberate.