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

Custom Map Types #1221

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

TimWhiting
Copy link

@TimWhiting TimWhiting commented Oct 8, 2022

@kevmoo
Iterated as requested on #997

Fixes: #396
See hacky workaround that is forced to do this at runtime for the fast_immutable_collections package, and is not able to handle enum keys:
https://github.com/marcglasberg/fast_immutable_collections/pull/25/files#diff-ba44d9486dfccb708bbdbbc167a5cfc096361c004eb01907d83fa4cfb2b8b889R1386

Custom map types with fromJson / toJson methods can now have keys automatically de/serialized to strings.

Usage:

Custom map types can make the fromJsonK / toJsonK parameters of their fromJson / toJson methods detected as map keys by using String? rather than Object? as the serialized type.

// Custom map type with custom serializers 
// (Example just wraps a map type, but imagine an immutable collections package)
class CustomMap<K, V> {
  final Map<K, V> map;

  CustomMap(this.map);

  factory CustomMap.fromJson(
    Map<String, dynamic> json,
    // Json serializable detects this as a custom Key value because of the (String?).
    K Function(String?) fromJsonK,  
    V Function(Object?) fromJsonV,
  ) =>
      CustomMap(json.map<K, V>(
          (key, value) => MapEntry(fromJsonK(key), fromJsonV(value))));

  Map<String?, dynamic> toJson( 
    // Json serializable detects this as a custom Key value because of the (String?).
    String? Function(K) toJsonK, 
    Object? Function(V) toJsonV,
  ) =>
      map.map((key, value) => MapEntry(toJsonK(key), toJsonV(value)));
}

// User of custom map type & JsonSerializable
@JsonSerializable()
class UseOfCustomMap {
  final CustomMap<int, String> map;

  UseOfCustomMap(this.map);

  factory UseOfCustomMap.fromJson(Map<String, dynamic> json) =>
      _$UseOfCustomMapFromJson(json);

  Map<String, dynamic> toJson() => _$UseOfCustomMapToJson(this);
}

Custom Map Types
@kevmoo
Copy link
Collaborator

kevmoo commented Oct 18, 2022

Hrm...I'm REALLY worried about the complexity added here – for a pretty narrow scenario.

@TimWhiting
Copy link
Author

TimWhiting commented Oct 19, 2022

Hrm...I'm REALLY worried about the complexity added here – for a pretty narrow scenario.

The complexity of auto-detection of when this applies, or the complexity of the implementation?

I actually thought that the implementation and auto-detection were relatively straightforward. Plumbing through a flag from the annotation might be less 'magic' to the user, but would require far more changes, and would require more cognitive burden on the end user of the collection type rather than the author.

As far as it being a narrow scenario.
The goal of json_serializable in my opinion is to automatically generate serialization methods for data models.
If this feature was trying to serialize objects that aren't data models, I would see the value in your argument.

I would argue that using immutable collections in your data model is a fairly common practice, (e.g. built_value being one that is used within google right?) and should be a use case we support just as much as the standard map.

@JoanSchi
Copy link

@TimWhiting

DateTime, BigInt url for example as key works fine without hacky workaround, because these types are implemented correctly, double int, bool are not parsed correctly I think that works for the hacky woraround.
See issue #1332. and issue: marcglasberg/fast_immutable_collections#58.

With DateTime I get a cast error because the DateTime is parsed correctly with toJsonK and then _safeKeyToJson tries to parse the already converted String into DateTime again in a DatateTime. -> error.

Maybe it is possible to use only hacky workaround for int double bool, until #1332 is fixed. :)

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.

Allow non-String map keys, if there exists a compatible converter
3 participants