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

Feature Request: in dump/safeDump support toJSON API #391

Closed
graphicore opened this issue Jan 19, 2018 · 12 comments
Closed

Feature Request: in dump/safeDump support toJSON API #391

graphicore opened this issue Jan 19, 2018 · 12 comments

Comments

@graphicore
Copy link

toJSON documented here for JSON.stringify:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#toJSON()_behavior

If an object being stringified has a property named toJSON whose value is a function, then the toJSON() method customizes JSON stringification behavior: instead of the object being serialized, the value returned by the toJSON() method when called will be serialized.

If dump and safeDump would support this, js-yaml would be better as a direct replacement for JSON. For example, Immutable provides toJSON. Could be optional, as far as I am concerned, so it's backwards compatible.

@puzrin
Copy link
Member

puzrin commented Jan 19, 2018

IMHO this is not related to yaml or i don't understand how this can work. Please provide more details or close issue.

Yaml dumper is to write yaml. If you need to write json, you probably need another package.

@graphicore
Copy link
Author

graphicore commented Jan 19, 2018

i don't understand how this can work. Please provide more details

Trying again 😸

Suppose I have an object myInstancewhich I want to serialize. Commonly I would do this in two steps:

  1. Write the data of my structure into simple JS objects and arrays myData = myInstance.getData();
  2. Serialize the data myJSON = JSON.stringify(myData) or myYAML = yaml.safeDump(myData)

However, JSON.stringify has a documented, standardized feature: if an object to serialize has a property called toJSON and if that property is callable, it will serialize the return value of toJSON.

This is handy, because it can spare me one step:

  1. Serialize the data myJSON = JSON.stringify(myInstance2);

example:

myInstance1 = {
  sayHi: function(){
    console.log('hello', this._name);
  },
  _name: 'world',
  getData: function() {
      return {name: this._name};
   }
}

myInstance2 = {
  sayHi: function(){
    console.log('hello', this._name);
  },
  _name: 'world',
  toJSON: function(){
      return {name: this._name};
   }
}

> JSON.stringify(myInstance1)
'{"_name":"world"}'

> JSON.stringify(myInstance2)
'{"name":"world"}'

Yaml dumper is to write yaml. If you need to write json, you probably need another package.

Since the behavior of JSON.stringify is documented in the ECMAScript standard, some projects are implementing toJSON in their libraries. A very popular example is immutable-js with ~ 22000 stars on Github.

I would like to use yaml.dump with objects that implement toJSON (namely immutable-js and likely also in my own project.)

current behavior:

// ignores toJSON
> yaml.safeDump(myInstance2, {skipInvalid: true})
'\n_name: world\n'

// ignores toJSON
> yaml.safeDump(myInstance2, {skipInvalid: true, toJSON: true})
'\n_name: world\n'

requested behavior:

// ignores toJSON
> yaml.safeDump(myInstance1, {skipInvalid: true})
'\n_name: world\n'

// uses toJSON!
> yaml.safeDump(myInstance2, {skipInvalid: true, toJSON: true})
'\nname: world\n'

NOTE: about skipInvalid: true I'm using it because of the sayHi method in my example. if toJSON: true and there are no other "unacceptable kinds" of objects, skipInvalid: true should not be needed.

@puzrin
Copy link
Member

puzrin commented Jan 19, 2018

IMHO it's better to keep things separate. If you know you can normalize object to json, then you can do:

yaml.safeDump(JSON.parse(your_obj.toJSON()))

This should work right now and does not require any changes.

@graphicore
Copy link
Author

Yeah, but that wouldn't work with deeper structures. toJSON would not run recursively.

IMHO it's better to keep things separate.

I think it's better to be input-compatible with JSON.stringify

@graphicore
Copy link
Author

graphicore commented Jan 19, 2018

example deeper serialization:

myInstance3 = {
  sayHi: function(){
    console.log('hello', this._name);
  },
  _name: 'world',
  toJSON: function(){
      return {name: this._name, deep: this.deep};
   },
   deep: {
      _value: 1234,
      toJSON: function(){
         return {value: this._value};
      }
   }
}

> JSON.stringify(myInstance3)
'{"name":"world","deep":{"value":1234}}'

Update with a more realistic example:

To illustrate with a more lifelike use case, suppose I have a tree-structure made from instances of Node

function Node(name) {
    this._name = name;
    this.children = [];
    
   // well, this is not exactly beautiful, apparently toJSON must be an ownProperty
    this.toJSON = this.toJSON;
}
Node.prototype.sayHi = function(){
    console.log('hello', this._name);
}
Node.prototype.toJSON = function() {
    return {name: this._name, children: this.children};
}


> parent = new Node('parent')
Node { _name: 'parent', children: [], toJSON: [Function] }
> parent.children.push(new Node('child1'), new Node('child2'));
2
> JSON.stringify(parent)
'{"name":"parent","children":[{"name":"child1","children":[]},{"name":"child2","children":[]}]}'

Now, I don't have to teach toJSON how to walk the tree, because JSON.stringify does that for me.

@graphicore
Copy link
Author

graphicore commented Jan 19, 2018

About your example, that's not how it works:

yaml.safeDump(JSON.parse(your_obj.toJSON()))

this would actually work (but it is madness):

yaml.safeDump(JSON.parse(JSON.stringify(your_obj)));

Please note: your_obj.toJSON() does NOT return a JSON-string (if that is not clear by now).

@puzrin
Copy link
Member

puzrin commented Jan 19, 2018

IMHO this package should work according to yaml spec, without magic. Your features are out of yaml spec. Hear are enougth problems without additional magic. If problem can be splitted to 2 independent stages, it's better to keep things separate. What do you think?

@graphicore
Copy link
Author

Your features are out of yaml spec.

The YAML spec does not cover that at all, but the JSON spec doesn't cover toJSON as well. Why should it? It's not the concern of either spec anyways.

If problem can be splitted to 2 independent stages ... What do you think?

I think toJSON is rather elegant and nice to have in JSON.stringify. Anyways, something like this could be used for a 2 stages process:

function prepareYAMLdump(obj) {
    var obj_, i, l, keys, key,
        replaceableTypes = new Set(['object', 'function']);

    if(obj === null || !replaceableTypes.has(typeof obj))
        return obj;

    if(Object.prototype.hasOwnProperty.call(obj, 'toJSON')
                                &&  typeof obj.toJSON === 'function')
        // run the replacer
        obj_ =  obj.toJSON();
    else
        obj_ = obj;

    keys = Object.keys(obj_);

    if(keys.length && obj_ === obj)
        // don't modify the original
        obj_ = Object.assign({}, obj);

    for(i=0,l=keys.length;i<l;i++) {
        key = keys[i];
        obj_[key] = prepareYAMLdump(obj_[key]);

    }
    return obj_;
}
yaml.safeDump(prepareYAMLdump(your_obj));

Though, enabling toJSON within yaml.dump would prevent the need of deep-copying the objects, because the serializer would just read the objects and prepareYAMLdump has to copy. Of course, 2 stages work, but it's a bigger effort than having it included, making JSON.stringify preferable in this regard.

@puzrin
Copy link
Member

puzrin commented Jan 23, 2018

The YAML spec does not cover that at all, but the JSON spec doesn't cover toJSON as well. Why should it? It's not the concern of either spec anyways.

Because there are no resources to write code just for fun :). When we follow spec - we know exactly where to stop. Without spec that can be endless investigations & iterations. I don't object against such feature, but will not participate myself :).

@graphicore
Copy link
Author

I'd like to know how toJSON got into JSON.stringify, who suggested it and where the discussion of it is.

but will not participate myself :)

That's totally OK. But, if I would implement it and PR it, it would still become part of the project and thus would add to the maintenance burden.

@graphicore
Copy link
Author

Apparently, this is a duplicate of #339

@puzrin
Copy link
Member

puzrin commented Jan 23, 2018

Could you then refer/move new info to existing issue and close this one?

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

No branches or pull requests

2 participants