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

Consider removing Codable requirement for State #45

Closed
CoreyKaylor opened this issue Sep 2, 2020 · 6 comments · Fixed by #46
Closed

Consider removing Codable requirement for State #45

CoreyKaylor opened this issue Sep 2, 2020 · 6 comments · Fixed by #46

Comments

@CoreyKaylor
Copy link

Is your feature request related to a problem? Please describe.
I'm using SwiftDux with a shared model in Kotlin-Native. This poses challenges with the Codable requirement.

Describe the solution you'd like
Ideally the requirement of having State be Codable should be removed as it is by nature an extension of the core.

Describe alternatives you've considered
Wrapping the primary state type with a wrapper that is Codable. This adds a lot of indirection and unnecessary complexity IMO.

@CoreyKaylor
Copy link
Author

Let me know if you're interesting in taking a PR for CoreyKaylor@3c87ecc or some variation of it.

@StevenLambion
Copy link
Owner

Hi @CoreyKaylor, I'm sorry for such a late response to this issue.

I completely agree that the codable constraint creates unnecessary friction. I'd be glad to take a look at the PR.

@StevenLambion
Copy link
Owner

Because StateType is more of a convenience and the Store class doesn't rely on it, I'm tempted to go in a direction that implements conditional protocol conformance in spots where codable and equatable might be required. In that case, the developer would be responsible to add the correct protocol on their state objects.

The reasoning behind this is the fact that the core classes don't require StateType, but other classes do when a conditional conformance would suffice. I can then mark StateType as deprecated.

Does this sound reasonable?

@CoreyKaylor
Copy link
Author

@StevenLambion I can relate to long delays in replying to issues, no worries there. I like your proposal over mine.

@StevenLambion StevenLambion linked a pull request Oct 14, 2020 that will close this issue
@StevenLambion
Copy link
Owner

I've pushed the change out to the latest release. I haven't marked StateType as deprecated yet, but the library is no longer constrained to it.

@CoreyKaylor
Copy link
Author

Looks great and works for the scenario I described. Thanks for the changes.

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 a pull request may close this issue.

2 participants