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

[WIP] Automatically convert custom Maps key types #997

Closed
wants to merge 1 commit into from

Conversation

TimWhiting
Copy link

@TimWhiting TimWhiting commented Oct 4, 2021

Before

Map<int, String> // works
IMap<int, String> // does not work

Custom toJson / fromJson map types can now have keys serialized to strings. This expects the to/fromJson function to be of the form String Function(Object?) and Object? Function(String).

Resolves: #396

@kevmoo

Copy link
Collaborator

@kevmoo kevmoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thought to add. This PR needs real tests. I'm not sure what's being fixed here!

@@ -26,6 +26,8 @@ Given a library `example.dart` with an `Person` class annotated with
[`JsonSerializable`]:

```dart
import 'dart:convert';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't add things to the default example!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted the changes to the example, and added an integration test which should help to show the use-case.

Custom Map Types
@TimWhiting
Copy link
Author

TimWhiting commented Jan 29, 2022

Addressed your comments.

The point of this PR is to allow custom implementations of from/toJson with generics to have key values automatically serialized to strings (such as ints, doubles, datetime, and enums). Keys were being serialized properly for built in map types, but not for custom maps such as the immutable map type I'm using from the fast_immutable_collections package. The approach I took was to reuse the key serialization logic that is used for regular maps in generic types that have String? types instead of the regular Object? type in the toJsonK/fromJsonK functions.

i.e.

class CustomMap<K, V> {
  final Map<K, V> map;

  CustomMap(this.map);

  factory CustomMap.fromJson(
    Map<String, dynamic> json,
    K Function(String?) fromJsonK,
    V Function(Object?) fromJsonV,
  ) =>
      CustomMap(json.map<K, V>(
          (key, value) => MapEntry(fromJsonK(key), fromJsonV(value))));

  Map<String?, dynamic> toJson(
          String? Function(K) toJsonK, Object? Function(V) toJsonV) =>
      map.map((key, value) => MapEntry(toJsonK(key), toJsonV(value)));
}

@FXschwartz
Copy link

@TimWhiting Would this make it possible to serialize Firebase Timestamps?

@TimWhiting
Copy link
Author

@FXschwartz
This PR is for custom container types with non-string keys into a string (with the same support as built in containers)

My understanding is that Firebase Timestamps are encoded as a json object.

{
  nanoseconds: 0,
  seconds: 1562524200
}

Are you asking for Firebase Timestamps as keys? If so, in order to be valid json you can serialize it to a string to make it a string key. Which is related but not what this PR is for. The current implementation only handles the same things that normal maps handle as keys (e.g. enums, int, DateTime, Uri).

If you had something else / more specific in mind maybe I can help you find the right issue for it.

@FXschwartz
Copy link

@TimWhiting Sorry for the delay in response! Yes, I'm talking about being able to define Firebase Timestamp as a key.

I was hoping your PR was at least a workaround to #1072 where if I'm using the Firestore ODM package, json_serializable can handle the parsing of a Firestore Timestamp.

Hopefully, that makes sense, and yes if you have any ideas that would be greatly helpful, but probably don't need to exist in this PR discussion.

@kevmoo
Copy link
Collaborator

kevmoo commented Oct 4, 2022

I'm going to close this out. Feel free to re-open if you'd like to keep cranking...

@kevmoo kevmoo closed this Oct 4, 2022
@TimWhiting TimWhiting mentioned this pull request Oct 8, 2022
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