clarkie / dynogels

DynamoDB data mapper for node.js. Originally forked from https://github.com/ryanfitz/vogels
Other
490 stars 110 forks source link

Item.toJS() instead of Item.toJSON()...? #9

Closed patoncrispy closed 8 years ago

patoncrispy commented 8 years ago

Should this function not be .toJS() rather than .toJSON() as it returns a JavaScript object, rather than JSON data?

clarkie commented 8 years ago

I'm not sure I understand. JSON stands for JavaScript Object Notation. What would you expect a function called .toJSON() to return?

patoncrispy commented 8 years ago

I would have expected a JSON string... It's semantics really and not that big a deal. :) I use the Immutable.js library and that has .toJS(), which I guess I'm used to so when I saw .toJSON() I was expecting a different result.

adrianblynch commented 8 years ago

I'm with @patoncrispy on this. This isn't right:

const myJson = {hello: 'peeps'} // I'm JS not JSON

Where as this is:

const myJs = {hello: 'peeps'} // JS
const myJson = JSON.stringify(myJs) // JSON, a string that can be parsed
const backToJs = JSON.parse(myJson) // We have JS
clarkie commented 8 years ago

How would you recommend transitioning to this? Also, toJS doesn't tell me what the function does.

What about toPlainObject()?

patoncrispy commented 8 years ago

Two possible suggestions:

  1. Change the function name from .toJSON() to .toJS().
  2. Introduce another function called .toJS(), which has the current functionality of .toJSON() and the make the original .toJSON() function return a JSON string.

To keep up with semver, you'd need to make a major release, as both of these suggestions are breaking changes.

Other than ImmutableJS (Facebook), the .toJS() syntax is also used by Knockout and I feel it makes a lot more sense when trying to retrieve an object than .toJSON() does. For instance, when I first found that function I used it like this:

const user = JSON.parse(User.toJSON()); 

This is the only reason I brought this up.

Mobx made this change to their API as well, so it could be worth looking at how they did it.

clarkie commented 8 years ago

Following up on this I found this in the MDN docs:

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

This means that there is a legitimate use for toJSON() that I didn't know about before.

patoncrispy commented 8 years ago

That's actually quite interesting. I didn't know that!