mickhansen / graphql-sequelize

GraphQL & Relay for MySQL & Postgres via Sequelize
MIT License
1.9k stars 172 forks source link

Document the semantics recognized by resolver's automatic support for order #584

Closed tomprogers closed 4 years ago

tomprogers commented 6 years ago

The documentation states that one of the benefits of using resolver is that it "Automatically converts an arg named 'order' to a sequelize order". But I can't find any supporting documentation for that feature:

These seemed like good guesses, but they failed:

query {
  dogList(order: ["breed", "desc"]) { // after all, sqlz takes an array of strings
    name
    breed
  }
}
query {
  dogList(order: "breed desc") { // seems straightforward
    name
    breed
  }
}

I did logging inside options.before to see how resolver was interpreting the order argument, and confirmed that it was receiving my argument, but that it was not interpreting it the way I expect.

In the end, I think I figured it out by digging through the source code.

I don't know where you got this reverse:FIELDNAME style, but it's not obvious, and it doesn't seem to originate in the GraphQL spec, either.

These details need to be documented, or devs will be unable to use them, and they'll eventually just ditch resolver in favor of hand-rolled resolve functions.

mickhansen commented 6 years ago

I agree that in retrospect, reverse: might not be ideal. In general i'm actually mostly against having any sort of magics, i think order statements should be defined as enums - Which the relay connection resolver has support for, maybe we could add something similar to the regular resolver.

tomprogers commented 6 years ago

reverse: seems fine. The reason I mentioned that it doesn't seem to reflect anything in the GQL spec was to show that it doesn't seem reasonable to expect anybody to already know about it, which increases the importance of documenting it in this project.

If it's a convention within Relay or Sequelize, it might be enough to simply link to the relevant documentation in those projects.

mickhansen commented 6 years ago

It's not a convention from anywhere, i don't think the GraphQL spec covers how to define order by statements, haven't read the latest revisions though.

In the beginning of this project a lot of "magic" stuff was merged, personally i'm moving more against explicitely defined stuff (atleast in terms of graphql schemas) as i think having strong types are one of the greatest benefits of GraphQL.

Documentation PRs always very welcome - It's easier for a new user to remember what they felt were missing.

tomprogers commented 6 years ago

I had written my own parsing that supported item( sort: "fieldname desc" ), gracefully ignoring the API value if the fieldname wasn't one of the fields on the type. But then I realized I could probably dig through the resolver source to figure out how to leverage the automatic order stuff.

Aukhan commented 5 years ago

@tomprogers pinging on an old issue to see if it can be clarified or closed. As far as I can tell resolver won't do any magic unless the order is specified as an argument, as part of defaultListArgs or separately. But I do agree that the reverse: part could have been specified in the docs as I had to look it up in the source code as well. gonna do a PR for that

Other than that as @mickhansen says that there isn't any GraphQL spec on how to define order by statements, one thought here is to maybe make it a JSONType and move it towards how we use it in sequelize, that way we could also specify more than one order clauses; order : [[ 'firstname', 'asc' ], [ 'id', 'desc' ]] this is obviously just a thought and a breaking change.

mickhansen commented 5 years ago

I personally like how we do it for relay connections, defining order by as an enum that then maps to sequelize order statements.

The reverse: prefix is definitely not ideal, but it was a natural fix to what ended up being an unideal design (exposing order directly)

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.