Support JsonSerializable(genericArgumentFactories: true) #696
Conversation
|
Sorry for the delay. I'll look into it |
| @@ -452,7 +452,7 @@ class Generic<T> with _$Generic<T> { | |||
| factory Generic(@DataConverter() T a) = _Generic<T>; | |||
|
|
|||
| factory Generic.fromJson(Map<String, dynamic> json) => | |||
| _$GenericFromJson<T>(json); | |||
| _$GenericFromJson<T>(json, (DataConverter<T>()).fromJson); | |||
There was a problem hiding this comment.
This currently is a negative change in my opinion.
Two ways to resolve this.
- Automatically detect JsonConverter annotations that handle generic types matching the surrounding class. (Very difficult)
- Explicit opt in / out of generic argument json factories, much easier, but unfortunate both if we make it opt in or opt out, and I'm not sure which I would prefer.
There was a problem hiding this comment.
What is the difficulty with 1 in your opinion?
There was a problem hiding this comment.
The fact that this is technically a breaking change bothers me.
I think we'd need to be opt-in.
There was a problem hiding this comment.
The difficulty is that you have to make sure that every parameter in every constructor has a type converter for any type recursively containing any generic type of the class, even for nested types! So a parameter that has type List<T> would need a type converter for List<T>.
Thanks, this is something I've wanted several times. This is a much cleaner approach than before, and rather simple, probably because of the rearchitecture of the freezed code you did recently. I've highlighted one issue that I see with my current solution. I'd like your opinion on how to proceed there. I've attempted the first solution separate from this PR, but I found it rather difficult to correlate type parameters on specific annotations of constructor arguments (for JsonConverters) to the generic parameter of the surrounding class. |
|
We'd need a matching parameter on the annotation and a config in the build.yaml file |
TimWhiting
force-pushed
the
master
branch
2 times, most recently
from
a00ebca
to
c6928ab
Compare
|
Updated to the opt-in solution with a parameter in the annotation and config in the build.yaml file. Also updated tests to exercise the new integration test objects. |
|
Could you add doc for this? Including the build.yaml config, which likely will be used often |
|
@rrousselGit Added to the readme |
|
Any more feedback, or can this get merged? |
|
Nothing to say, thanks for your work! |
New attempt at resolving #370
fixes: #331