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

Should the Keystone API conform to the http://jsonapi.org/ spec? #558

Closed
davidbanham opened this issue Aug 21, 2014 · 12 comments
Closed

Should the Keystone API conform to the http://jsonapi.org/ spec? #558

davidbanham opened this issue Aug 21, 2014 · 12 comments

Comments

@davidbanham
Copy link
Contributor

Carrying on discussion from here:

#404

Should Keystone API aim to conform to this spec? Right now, the default list API returns data that looks like the following:

[
  {
    "slug": "foobar",
    "title": "foobar",
    "author": "538ef22e3bc7438c4f736cc4",
    "_id": "538ef48c190930f85218f8a7",
    "__v": 0,
    "createdAt": "2014-06-04T10:27:24.612Z",
    "state": "draft"
  },
  {
    "__v": 0,
    "_id": "53b4fbaab552fb00002f3653",
    "slug": "sdas",
    "title": "sdas",
    "content": {
      "brief": "<p>asd</p>",
      "extended": "<p>asd</p>"
    },
    "createdAt": "2014-07-03T06:43:54.000Z",
    "state": "draft"
  }
]

The only significant difference that I can see is that the jsonapi spec asks for the root element to be a dict and the List API returns an array. I know Angular plays more nicely with dicts than arrays, and @Globegitter has indicated the same about Ember.

I have also been burned by root element arrays in the past. Nothing insurmountable, but multiple times I have said to myself "I really want to add a new property to this response, and the fact that it's just an array is making that a pain in the bum. Now I have to update all the consumers of this endpoint to expect a dict."

A real-world use case I've had this on has been adding a flash messaging system. I was writing the Angular frontend for a Sinatra server, and we wanted the ability to add arbitrary flash messages to all API responses. I wrote a bit of middleware in the Sinatra app that appended something like the following to all responses:

{
  "data": ["super important client stuff"],
  "flash": [
    {
      "message": "You're pretty great!",
      "level": "info"
    }
  ]
}

And then some middleware in the Angular app that checked every API call for a conformant flash property and, if it found one, displayed any messages to the user.

Obviously, this could only be done on endpoints that returned dictionaries. Endpoints that returned arrays were not compatible.

So, should we change all list API calls so that they conform?

@davidbanham davidbanham mentioned this issue Aug 21, 2014
@JedWatson
Copy link
Member

I'm 100% on board with this. Arrays at the top level are a nightmare.

I like the way the jsonAPI doesn't enforce what the main results array is called; probably from generic requests I'd call it results rather than the model key, but I could be swayed on this.

I also like the way they've defined links; it's pretty scalable, although when left to my own devices I usually embed the data I need as populated by mongoose. So maybe we can be flexible on this.

@davidbanham
Copy link
Contributor Author

The links are optional, so I think we can kick that decision down the line.

I agree with calling it something generic rather than the name of the model. I was originally thinking data but I like results better.

This will be a compatibility breaking change, though. Do we have any reason not to ship a new major version at the minute?

@JedWatson
Copy link
Member

Results is consistent with our pagination stuff (and probably a few other areas).

This and the express 4 stuff are breaking changes so we can roll them together. I was planning to release 0.3.0 with the new React components in... oh, about a week?

No reason not to ship two major versions in rapid succession I guess...

@ignlg
Copy link

ignlg commented Aug 21, 2014

To follow a well documented spec is always appreciated.

@davidbanham
Copy link
Contributor Author

I think I mashed the keyboard. Apologies.

@JedWatson
Copy link
Member

This may be my favorite keystone-issues moment yet. Such confusion. There's no what, David???

@davidbanham
Copy link
Contributor Author

That was going to be "There's no real reason not to ship two majors so close together, but it can be confusing to users of the project. Nobody likes to have to think about two backwards compatible changes within a week. A part of me wants to ship it, ship it now, but another part of me things if it's only a short time we may be better off waiting. Maybe set a hard cap on the max time we're willing to wait for the React stuff."

But then I got distracted and wandered off, but somehow hit Close and comment.

@Globegitter
Copy link
Contributor

Haha - I like how Github is now showing comments and activity in real time. Made writing this quite a bit more fun.
Anyway, I think there is nothing against embedded data - not that I think Ember is the golden standard, but they just added default support for embedded data, so there seems to be some other clever people thinking similarly ;)
And in terms of naming the root element like the model key, the way I see it, is that you have a model and all is named coherently you don't have to think about things much and I think it also makes it all nicely readable. So you have a model Post, as a default you get all/multiple of them from the /posts route, a single one from /posts/:id and then you just load the posts array or post object respectively. And you know exactly what response you are getting. This might not be a debate for here and this of course your project @JedWatson but those are my 2 cents about naming the results element.

@davidbanham
Copy link
Contributor Author

It makes conceptual sense to have the property of each response named according to the type of thing you're getting, but I find you hit some practical issues with it.

The main thing that bugs me is it makes it more difficult to write code that abstracts across all kinds of responses.

The point I would raise is, what does having the property named according to the model gain us? I can't see a distinct benefit, but I can see negatives.

@JedWatson
Copy link
Member

I've had mixed experiences with dynamic root properties too, it adds one more thing you have to know when processing results with a generic handler.

If we're going to conform, I just noticed we're supposed to call it data, not results, which I think is a bit of a shame... but I can live with it if our goal is to meet the spec.

Of the tools available that can work with it (Angular, Ember, Backbone, etc) I would expect most have the ability to easily customize the structure of the API, so I'm not sure yet whether we should use it as a guide or conform to it exactly, with regards to things like the restrictions on the top level.

Votes? Establish our own API based on the spec, or match it to the letter?

After reading more of it, I think following but not matching it will suit us better, there are some things that conflict with the way Mongoose / Keystone does things (e.g. handling of to-many relationships, i.e. Relationship fields) which will require a fair bit of weirdness in the implementation to meet (i.e. nesting relationship fields under a links object, instead of just including the schema as-is)

re: bumping major versions, two in rapid succession is probably worse for upgraders, let's take the opportunity to finalize the rest of the decisions around an API as well (including my favorite issue number, #404)

@creynders
Copy link
Contributor

(Triage note: I think this is a really valid discussion; leaving it open)

@JedWatson
Copy link
Member

I think at this point, the ship has sailed. Keystone 5 is using GraphQL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants