Skip to content
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

Improve the API for choosing the type of serialization #162

Open
sindresorhus opened this issue Feb 3, 2024 · 10 comments
Open

Improve the API for choosing the type of serialization #162

sindresorhus opened this issue Feb 3, 2024 · 10 comments

Comments

@sindresorhus
Copy link
Owner

Current:

extension Foo: Defaults.Serializable, Defaults.PreferRawRepresentable {}

I think this may be better:

extension Foo: Defaults.Serializable.RawRepresentable {}

This is shorter, but more importantly, it makes the choice explicit. Users should prefer this over Defaults.Serializable. Defaults.Serializable has a big flaw. For example, you have a struct X: Codable, Defaults.Serializable. You save data with it, but later on, you add a RawRepresentable conformance. The previous persisted version of the struct can no longer be loaded as it's saved using the Codable bridge, but now tries to load using the RawRepresentable bridge.

// @hank121314 Thoughts? Any better way to prevent this problem?

@hank121314
Copy link
Collaborator

You save data with it, but later on, you add a RawRepresentable conformance. The previous persisted version of the struct can no longer be loaded as it's saved using the Codable bridge.

For this case, we could consider enhancing the RawRepresentableCodableBridge.
Since its Value conforms to both Codable and RawRepresentable, we could test which deserialization can be loaded.
If the user switches the protocol from Codable to RawRepresentable, there may be a need for some migration.

@sindresorhus
Copy link
Owner Author

Since its Value conforms to both Codable and RawRepresentable, we could test which deserialization can be loaded.

A value could potentially be loadable by both. This creates ambiguity and potential data-loss.

If the user switches the protocol from Codable to RawRepresentable, there may be a need for some migration.

Yes, that's a different case and unrelated to this issue.

@hank121314
Copy link
Collaborator

hank121314 commented Feb 10, 2024

A value could potentially be loadable by both. This creates ambiguity and potential data-loss.

How about change the deserialize function in RawRepresentableCodableBridge to this?

public func deserialize(_ object: Serializable?) -> Value? {
		// Check whether it can deserialize with `CodableBridge`
		if 
			let jsonString = object as? String,
			let value = try? Value(jsonString: jsonString) 
		{
				return value
		}
		guard let object = object as Value.RawValue else {
			return nil
		}
		return Value(rawValue: object)
}

It will accept serialization values from both CodableBridge and RawRepresentableBridge.

@sindresorhus
Copy link
Owner Author

sindresorhus commented Feb 11, 2024

To clarify my point. The potential problem is that the raw serialized value could be a string from a RawRepresentable serialization, but when deserialized using Codable it turns into a different value. I don't think we should guess the user's intent. It's better to be explicit about what you want.

In addition, trying to use Codable each time deserialize() is called would add a lot of unnecessary overhead, since Codable is slow.

@hank121314
Copy link
Collaborator

Get your point.
I think we might need to allow the user to choose the Bridge they want to use for serialization and deserialization.
Here are the few solutions I come up:

  1. Remove all ambiguous conformance in Defaults, and expose the internal bridge publicly to allow users to choose it.
private enum FixtureCodableEnum: String, Hashable, Codable, Defaults.Serializable {
	case tenMinutes = "10 Minutes"
	case halfHour = "30 Minutes"
	case oneHour = "1 Hour"
}

extension FixtureCodableEnum {
	typealias Bridge = Defaults.RawRepresentableCodableBridge<Self>;
}
  1. Check conformance during Defaults.Key.init and issue an Xcode warning advising users to utilize Defaults.Serializable.RawRepresentable instead.
// in `Defaults.Key.init`
if defaultValue is Codable, defaultValue is any RawRepresentable {
	runtimeWarn(false, "Please consider using `Defaults.Serializable.RawRepresentable` for serialization/deserialization stability")
}

@sindresorhus
Copy link
Owner Author

Why not?

private enum FixtureCodableEnum: String, Hashable, Codable, Defaults.Serializable.RawRepresentable {
	case tenMinutes = "10 Minutes"
	case halfHour = "30 Minutes"
	case oneHour = "1 Hour"
}

@hank121314
Copy link
Collaborator

Why not?

Yeah, of course we can do this, but we might need to warn user when they are using ambiguous bridge to do the serialization and deserialization. With this user will need to specify the bridge they want to use.

@sindresorhus
Copy link
Owner Author

I think 2 would be the best choice.

@hank121314
Copy link
Collaborator

I think 2 would be the best choice.

Got it! Many thanks!
With option 2, I think there might be a need to also having a Defaults.Serializable.Codable?

@sindresorhus
Copy link
Owner Author

With option 2, I think there might be a need to also having a Defaults.Serializable.Codable?

Yes. And Defaults.Serializable.NSSecureCoding.

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

No branches or pull requests

2 participants