Skip to content

Add support for Codable types to AppStorageKey #182

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

Merged
merged 1 commit into from
Aug 11, 2025

Conversation

abdulilah99
Copy link
Contributor

I understand you’ve previously noted that Codable support is debatable, but adding it could significantly broaden the library’s range of use cases, I hope that makes it an appealing prospect for you.

@stephencelis
Copy link
Member

@abdulilah99 Thanks for the nudge! We've let this sit long enough that we think support is probably worth the trade-offs. I'm going to merge as-is but will add some follow up documentation about the caveats.

@stephencelis stephencelis merged commit a541c5b into pointfreeco:main Aug 11, 2025
5 of 6 checks passed
@pyrtsa
Copy link
Contributor

pyrtsa commented Aug 11, 2025

I'm a bit concerned about this change. There are so many .appStorage("foo") overloads now, that from the call site, it may not be clear whether the value is persisted as JSON-serialised Data or one of the native types. 🤔

I think it would be less magical, for example, if key was a labelled parameter such as .appStorage(json: "foo") or .appStorage(codable: "foo") (just spitballing here).

@stephencelis
Copy link
Member

stephencelis commented Aug 11, 2025

@pyrtsa We considered that but don't think it's a problem in practice, and we have plenty of tests showing that it does the right thing with more primitive types. We also couldn't come up with an example of things going wrong. @AppStorage is almost always going to be used in a concrete way where the appropriate overload will be resolved.

The one place where this might not be the case is if someone writes a wrapper around @AppStorage that only implements Codable support and then tries to access a value as a primitive using @AppStorage. Such an edge case seems more like a programmer error than anything else.

@pyrtsa
Copy link
Contributor

pyrtsa commented Aug 11, 2025

@stephencelis But primitive types are only part of the picture. With #107, it's not just primitive types but [String] too which, going via stringArray(forKey:), is just a special case of array(forKey:). Why does [String] go that way but [Int] turns into a JSON-serialised Data instead?

Also, there's no way to customise the JSONEncoder and JSONDecoder that get used. One might want to store Dates in a special way, for instance, but there's no way to change the dateEncodingStrategy. Nor is there a way to use another serialisation than JSON.

My worry is partly about the library painting itself into a corner here. Something that's completely avoidable with just a little bit more qualification to keep structured types isolated from the primitive types supported by AppStorage.

@stephencelis
Copy link
Member

But primitive types are only part of the picture. With #107, it's not just primitive types but [String] too which, going via stringArray(forKey:), is just a special case of array(forKey:). Why does [String] go that way but [Int] turns into a JSON-serialised Data instead?

Because UserDefaults special-cases the same API, and it'll be slightly more performant than going through Codable machinery.

Also, there's no way to customise the JSONEncoder and JSONDecoder that get used. One might want to store Dates in a special way, for instance, but there's no way to change the dateEncodingStrategy. Nor is there a way to use another serialisation than JSON.

I think both of these issues could be addressed the same way they are addressed in fileStorage. It'll just require some time to PR for folks that need it.

My worry is partly about the library painting itself into a corner here. Something that's completely avoidable with just a little bit more qualification to keep structured types isolated from the primitive types supported by AppStorage.

Can you qualify this worry a bit more with examples? I think the issues you brought up above are all addressable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants