launchdarkly / flutter-client-sdk

LaunchDarkly Client-side SDK for Flutter mobile applications
Other
15 stars 17 forks source link

Handle conversions between LDValue and Dart's primitive types #133

Closed danallen88 closed 6 months ago

danallen88 commented 9 months ago

Is your feature request related to a problem? Please describe. It would be great to see the SDK handle conversion of feature flag values to Dart's primitive types transparently so that our client code does not need to integrate with the LDValue wrappers. With the 4.x update, LDValue is final, meaning it cannot be mocked. This isn't a huge deal, since it's a relatively simple interface. The bigger issue is that in order to avoid leaky abstractions such as passing around LDValue in our client code in a layered architecture, we must convert LDValues to Dart primitives ourselves.

Describe the solution you'd like Ideally, since we already have to have knowledge of what value type the feature flag is since we must ask for the specific variation, the *Variation methods should just return a Dart primitive such as a bool for boolVariation, Map<String, dynamic> for jsonVariation, and so on.

Describe alternatives you've considered Because we have to wrap/unwrap these values ourselves, this leads to some complexity which I believe must be encountered by other developers as well that do not wish to depend upon LDValue in layers above a data client layer. However, even this has drawbacks. For example: LDValueType contains an entry for number, but does not contain LDValueType.int and LDValueType.double. This is problematic for conversions because in the end, I still have to get the variation by intVariation or doubleVariation, but if I'm looking at a single LDValue, it can only tell me if it is a LDValueType.number.

Additional context Consider the following code:

/// {@template launch_darkly.json_variation}
  /// Gets a JSON variation from LaunchDarkly for a given [flagKey].
  ///
  /// The [defaultValue] will be returned if a connection to LaunchDarkly cannot
  /// be established, the flag does not exist, or an error occurs.
  ///
  /// If the flag called is not intended to return JSON values, this will
  /// instead return an empty map.
  /// {@endtemplate}
  Map<String, dynamic> jsonVariation({
    required String flagKey,
    required Map<String, dynamic> defaultValue,
  }) {
    try {
      final flagValue = _ldClient.jsonVariation(
        flagKey,
        _convertToLDValue(defaultValue),
      );

      return flagValue.type == LDValueType.object
          ? _convertLDValueToMap(flagValue)
          : {};
    } catch (error, stackTrace) {
      Error.throwWithStackTrace(
        LaunchDarklyClientFlagException(error),
        stackTrace,
      );
    }
  }

We use this method to wrap LDClient's jsonVariation method to ensure that we don't leak LDValue outside of our wrapper client. This requires an implementation that can recursively convert a JSON object to an LDValue and vice versa, which could look like:

dynamic _convertFromLDValue(LDValue value) {
    switch (value.type) {
      case LDValueType.nullType:
        return null;
      case LDValueType.boolean:
        return value.booleanValue();
      case LDValueType.number:
        return value.doubleValue();
      case LDValueType.string:
        return value.stringValue();
      case LDValueType.array:
        return _convertLDValueToList(value);
      case LDValueType.object:
        return _convertLDValueToMap(value);
    }
  }

  List<dynamic> _convertLDValueToList(LDValue list) {
    assert(list.type == LDValueType.array, 'Value must be LDValueType.array');

    return [
      for (final value in list.values) _convertFromLDValue(value),
    ];
  }

  Map<String, dynamic> _convertLDValueToMap(LDValue value) {
    assert(
      value.type == LDValueType.object,
      'Value must be LDValueType.object',
    );

    return {
      for (final key in value.keys) key: _convertFromLDValue(value.getFor(key)),
    };
  }

  LDValue _convertObjectToLDValue(Map<String, dynamic> map) {
    final objectBuilder = LDValue.buildObject();
    for (final key in map.keys) {
      final value = map[key];
      objectBuilder.addValue(key, _convertToLDValue(value));
    }
    return objectBuilder.build();
  }

  LDValue _convertToLDValue(dynamic value) {
    if (value == null) {
      return LDValue.ofNull();
    }

    if (value is num) {
      return LDValue.ofNum(value);
    }

    if (value is String) {
      return LDValue.ofString(value);
    }

    if (value is bool) {
      return LDValue.ofBool(value);
    }

    if (value is Map<String, dynamic>) {
      return _convertObjectToLDValue(value);
    }

    if (value is List) {
      return _convertListToLDValue(value);
    }

    throw LaunchDarklyClientTypeException(
      'Cannot convert unsupported type to LDValue: ${value.runtimeType}',
    );
  }

  LDValue _convertListToLDValue(List<dynamic> list) {
    final listBuilder = LDValueArrayBuilder();
    for (final value in list) {
      listBuilder.addValue(_convertToLDValue(value));
    }
    return listBuilder.build();
  }

Another downside of this is that Lists should contain only a single type, so a better implementation would introduce a type parameter and use generics, but hopefully this illustrates the point.

kinyoklion commented 9 months ago

I think we can look into adding something which allows for relatively simple conversion. Internally LDValue types can be converted to JSON, which takes the form of dynamic, Map<String, dynamic> or List<dynamic>.

We can look into generics as well. I think we could actually support any type for which you could provide a fromJson implementation in a relatively elegant way.

Thank you, Ryan

Filed internally as 234344

kinyoklion commented 8 months ago

This relates to: https://github.com/launchdarkly/flutter-client-sdk/pull/134

It doesn't yet provide support for generics, but it does allow easy conversion to/from basic types with dynamic.

kinyoklion commented 8 months ago

I've not added generics to do conversions, but the support for to/from dynamic is released in 4.2.0. I think this should be sufficient for the original request, and then we can improve the ergonomics potentially in the future.

A note with doubles versus ints. Numbers in LaunchDarkly are always doubles, because they are conveyed as JSON numbers. If you want to access a number as an integer or double, then it is safest to use (thing as num) then use toInt or toDouble to get the type you want.

Note the dynamic can be just a singlular dynamic, a Map<String, dynamic> or a List<dynamic>. So if you are using a fromJson method, then you may need to do a check like thing is Map<String, dynamic>. Whatever shape your json method expects.

kinyoklion commented 6 months ago

I am going to close this request now. I think we covered the original use case. If there is a desire for generics specifically, then a new issue could be created for that.

Thank you, Ryan