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

Replace directives with annotations #62

Open
danielrearden opened this issue May 21, 2020 · 2 comments
Open

Replace directives with annotations #62

danielrearden opened this issue May 21, 2020 · 2 comments
Labels
enhancement New feature or request
Milestone

Comments

@danielrearden
Copy link
Owner

The library currently makes use of schema directives for both transforming the schema and providing metadata about the underlying database. This works well enough, but will make it difficult to integrate Sqlmancer with code-first libraries (or for that matter, vanilla GraphQL.js) without publishing plugins specific to those libraries. Schema directives were never meant to be used for relaying metadata and using them this way has been demonstrably fragile, with AST information being "lost in translation" in various contexts.

Putting annotations inside descriptions (which could then be stripped when transforming the schema), would address these points. The schema might be a bit more verbose this way, but in some ways it would also be cleaner since all the info would just be moved above the type or field instead of getting shoved inline.

Compare:

type Query
  @sqlmancer(
    dialect: POSTGRES
    transformFieldNames: SNAKE_CASE
    customScalars: { JSON: ["JSON", "JSONObject"], Date: ["DateTime"] }
  ) {
  actors: Actor @paginate @many(model: "Actor")
}

with

"""
@sqlmancer {
  dialect: 'POSTGRES',
  transformFieldNames: 'SNAKE_CASE',
  customScalars: { JSON: ['JSON'], Date: ['DateTime'] },
}
"""
type Query {
  """
  @paginate
  @many { model: 'Actor' }
  """
  actors: Actor
}

The biggest hurdle with this approach is that there is no standard way of doing annotations like this. Whatever syntax is used needs to support multi-line strings (for example, for CTE definitions), so existing libraries like graphql-metadata and graphql-annotations are out.

@danielrearden danielrearden added the enhancement New feature or request label May 21, 2020
@danielrearden danielrearden added this to the Possible features milestone May 21, 2020
@danielrearden
Copy link
Owner Author

Initial pass available in this branch.

@yanickrochon
Copy link

I do not understand why passing dialect to the query object. Isn't that the role of each model? Esepcially if multiple clients are to be supported? And the goes with transformFieldNames. In short :

"""
@sqlmancer {
  customScalars: { JSON: ['JSON'], Date: ['DateTime'] },
}
"""
type Query {
  """
  @paginate
  @many { model: 'Actor' }
  """
  actors: Actor
}

The dialect is inferred by the model's Knex instance (see issue #15) and whether to convert the field names with a special case should be specified per model definition, not the actual query. Because if the type Query definition has many different models, connected to different tables and databases, then setting these properties at the Query level violates the contract of that definition; each model is in charge of their own dialects and field transformations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants