Serializing/deserializing methods for Event instances

[eliasyishak]

Fixes:

• Helper method to turn JSON into an Event instance #250

Usage workflow

/// Serializing
final event = Event.analyticsException(
  workflow: 'workflow',
  error: 'error',
  description: 'description',
);

print(event.toJson());
// {
//     "eventName": "analytics_exception",
//     "eventData": {
//         "workflow": "workflow",
//         "error": "error",
//         "description": "description"
//     }
// }

/// Deserializing
final jsonString = '''
{
    "eventName": "analytics_exception",
    "eventData": {
        "workflow": "workflow",
        "error": "error",
        "description": "description"
    }
}
''';

final constructedEvent = Event.fromJson(jsonString);

---

• I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

• See our contributor guide for general expectations for PRs.
• Larger or significant changes should be discussed in an issue before creating a PR.
• Contributions to our repos should follow the Dart style guide and use dart format.
• Most changes should add an entry to the changelog and may need to rev the pubspec package version.
• Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

[kenzieschmoll]

instead of creating our own, can we use something from package:collection? like DeepCollectionEquality?

[eliasyishak]

Good idea, this commit has it updated to use DeepCollectionEquality.equals to compare instead

@christopherfujino, i know we discussed trimming down dependencies for this package, do you think this is a valid reason to add another dep?

[eliasyishak]

Following up on offline discussions with @christopherfujino, going to remove the dep on collection and use the same function for comparing two separate maps to reduce dependencies for this package

[kenzieschmoll]

I think you can simplify this using pattern matching:

if (jsonMap
    case  {
      'eventName': final String eventName, // you might be able to go further here to validate the DashEvent check in the pattern
      'eventData': final Map<String, Object?> eventData,
    }) {
  // Retrieve the correct DashEvent enum from the provided label
  final dashEvent = DashEvent.getDashEventByLabel(eventName);
  if (dashEvent == null) {
    return null;
  }
  return Event._(eventName: dashEvent, eventData: eventData);
}

[eliasyishak]

Oh nice, this is a clever, had to catch up on Craigs pattern matching video to understand the syntax but it looks like this use case is highlighted pretty much exactly!

https://www.youtube.com/watch?v=aLvlqD4QS7Y&ab_channel=Flutter

Thank you for the suggestion! :)

[eliasyishak]

(Still digging through if this is possible) but i was able to simplify by adding a when clause to the case as the below

  static Event? fromJson(String json) {
    try {
      final jsonMap = jsonDecode(json) as Map<String, Object?>;

      // Ensure that eventName is a string and a valid label and
      // eventData is a nested object
      if (jsonMap
          case {
            'eventName': final String eventName,
            'eventData': final Map<String, Object?> eventData,
          } when DashEvent.getDashEventByLabel(eventName) != null) {
        return Event._(
          eventName: DashEvent.getDashEventByLabel(eventName)!,
          eventData: eventData,
        );
      }

      return null;
    } on FormatException {
      return null;
    }
  }

But it seems like i would have to make a call to DashEvent.getDashEventByLabel twice, do you know if its possible to assign this variable AND have it get checked within the when clause?

If it's not possible, i'll probably remove the when clause and leave the check within the body of the if

[christopherfujino]

drive by comment--at least with this particular code, I think the solution is to pull DashEvent.getDashEventByLabel(eventName) into a local variable before the if statement.

[eliasyishak]

I wouldn't have access to that variable above the if statement though because it gets assigned in the if-case statement.. unless we don't check if it is a string first

[christopherfujino]

ahh yeah, I see what you mean. carry on :)

[kenzieschmoll]

Ah yeah I'm now sure if DashEvent.getDashEventByLabel(eventName) can be stored in a variable. Maybe @munificent knows?

[munificent]

There isn't always a great way to interleave a series of pattern matches and guards so that you can incrementally bind stuff after a guard passes. In this case, you could do:

if (jsonMap
    case {
      'eventName': final String eventName,
      'eventData': final Map<String, Object?> eventData,
    }) {
  if (DashEvent.getDashEventByLabel(eventName) case var event?) {
    return Event._(eventName: event, eventData: eventData);
  }
}

Whether you like seeing nested if-case statements like this is a matter of taste. :)

If you wanted to really jam everything into a pattern, you could make DashEvent.getDashEventByLabel an extension on String:

extension DashEventExtensions on String {
  // TODO: Whatever the real return type should be.
  Object? get dashEventForLabel => ...
}

You can call extension getters in object patterns, so that would let you do:

if (jsonMap
    case {
      'eventName': String(dashEventForLabel: var event?),
      'eventData': final Map<String, Object?> eventData,
    }) {
  return Event._(
    eventName: event,
    eventData: eventData,
  );
}

Then the pattern will only match if the map's 'eventName' key is mapped to a string where calling dashEventForLabel on it returns a non-null value. When that happens, the value is bound to event.

But this feels to me like twisting the language out of shape and I wouldn't do it (though it is kind of cool that you can).

[munificent]

We've discussed making case an expression instead of directly built into the if-case statement. If we did that, then you could probably chain them using && like:

if (jsonMap
    case {
      'eventName': final String eventName,
      'eventData': final Map<String, Object?> eventData,
    } &&
    DashEvent.getDashEventByLabel(eventName) case var event?) {
  return Event._(eventName: event, eventData: eventData);
}

But that's not in the language now and it's not clear if it's a good idea. It raises lots of tricky questions around the scope of the pattern variables.

[eliasyishak]

@munificent yeah looks like your example in the second comment would be ideal but i can see how it get tricky.. i went with just having a separate conditional within the body of the if-case so it's easy to understand, thank you for the thorough comment!

[kenzieschmoll]

nit: use the simplified syntax from above for consistency
DashTool.values.where((t) => t.label == label).firstOrNull;

[christopherfujino]

LGTM