wrapAsYamlNode should maybe call .toJson()

[jonasfj]

I think the idea with wrapAsYamlNode was that you could pass it any value that json.encode (from dart:convert) would accept, and then this would wrap it as a YamlNode.

However, reading docs for jsonEncode I realize that unless some toEncodable function is given, json.encode will default to calling .toJson() on objects. This is in turn used by package:json_serializable which generates code that helps users add a .toJson() on their custom objects, making it possible to pass these objects to json.encode.

I wonder if we should consider calling .toJson() instead of throwing:

Invalid argument (value): Not a valid scalar type!: Instance of MyCustomObject
  package:yaml_edit/src/utils.dart 51:3   assertValidScalar
  package:yaml_edit/src/wrap.dart 73:5    wrapAsYamlNode
  package:yaml_edit/src/wrap.dart 126:28  new YamlMapWrap
  package:yaml_edit/src/wrap.dart 68:12   wrapAsYamlNode
  package:yaml_edit/src/wrap.dart 126:28  new YamlMapWrap
  package:yaml_edit/src/wrap.dart 68:12   wrapAsYamlNode

---

If we do this, we should certainly document it, and probably give an example. I doubt it is possible to gracefully test if the object has a .toJson() method, even accessing the property like object.toJson is Function is liable to throw and complain that no getter exists for toJson (if the method isn't present).

EDIT: It seems json.encode just catches all errors / exceptions when calling .toJson and then rethrows them as an exception saying: Converting object to an encodable object failed: Instance of MyCustomObject.

We could also consider adding a toEncodable function as an optional parameter similar to jsonEncode.

@walnutdust, any thoughts?

[walnutdust]

hmm... I'm not too sure I see the utility of switching over to toJson(). The default toJson() essentially looks at the object, determines if its a List/Map/bool/string or some other primitive type to encode it in a string, and we are already doing something analogous in wrapAsYamlNode. I also think that having the standard behaviour depends on toJson(), when JSON is a separate (though compatible!) format and getting that to work with package:json_serializable might feel kind of weird. Furthermore, toJson converts it to a string, and the resulting YAML might not fit the expectations?

It seems to me as though this change will only be valuable if for some reason the user is working with both JSON and YAML representations of their data and encoding them separately. In the cases where the user is expecting to save representations of their custom objects in various configuration files, it might be better for the user to call the toJson() on their side to obtain a representation that is entirely in terms of List/Map/bool/string/etc for the various configuration languages rather than expecting that to be the default behavior?

toEncodable() might provide some convenience, but I'm not sure if it's worth modifying the library for? I think the main argument for adding a toEncodable() for us would be to help us achieve an API that is similar to json.encode, but the user could simply have the encoding function as a class function and call it before passing their object to us.

@jonasfj thoughts?

[jonasfj]

Furthermore, toJson converts it to a string, and the resulting YAML might not fit the expectations?

No it converts to something "encodable".

The following sample actually works:

import 'dart:convert';

class CustomThing {
  String value;
  CustomThing(this.value);
  
  Object toJson() => <String,Object>{'value': value};
}

void main() {
  final data = {
    'myThing:': CustomThing("hello world"),
    'otherKey': 42,
  };
  print(json.encode(data));
}

But if instead of using json.encode I try to do: wrapAsYamlNode(data) that will throw.

The convention appears to be that custom objects with a .toJson() method, can be encoded by json.encode. I'm suggesting that we consider allowing wrapAsYamlNode to wrap custom objects that have a toJson() method.

Alternatively, anyone who wants to do this, can just do: wrapAsYamlNode(json.decode(json.encode(data))), and that'll work perfectly fine :D

We could also make the convention that wrapAsYamlNode will try to call .toYaml() and if that doesn't exist, then try to call .toJson().

toEncodable() might provide some convenience, but I'm not sure if it's worth modifying the library for? I think the main argument for adding a toEncodable() for us would be to help us achieve an API that is similar to json.encode, but the user could simply have the encoding function as a class function and call it before passing their object to us.

Well, if you have a hierarchy of custom objects, or you are storing custom objects in a Map<String,CustomThing> then you would need to write a method to convert that too... toEncodable gets called for all elements in tree, unless it's already encodable (ofcourse). Another way to do my sample above is:

import 'dart:convert';

class CustomThing {
  String value;
  CustomThing(this.value);
}

void main() {
  final data = {
    'myThing:': CustomThing("hello world"),
    'otherKey': 42,
  };
  print(json.encode(data, toEncodable: _toEncodable));
}

Object _toEncodable(Object o) {
  if (o is CustomThing) {
    return <String,Object>{'value': o.value};
  }
  return o; // I give up
}

Again, I don't have to write something to convert Map<String,dynamic> or Map<String,CustomThing> or List<CustomThing>, just something that converts CustomThing into something that is encodable as JSON.

My suspicion is that something like toEncodable and toJson would be convenient for wrapAsYamlNode.

[walnutdust]

ah, my bad, I got the json.encode function mixed up with toJson. Thank you for providing the examples and all! Those do sound like reasonable enhancements that will help wrapAsYamlNode, and I'll work on them soon! :)

[jonasfj]

Closed as package:yaml_edit has moved to dart-lang/yaml_edit.

Issue was refiled as dart-lang/yaml_edit#1