Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

improve query ergonomics #31

Closed
wants to merge 13 commits into from
Closed

improve query ergonomics #31

wants to merge 13 commits into from

Conversation

bnchdrff
Copy link
Member

@bnchdrff bnchdrff commented Jan 29, 2019

fixes #21
fixes #9

use custom resolvers to offer enum argument types

offer "meta" org fields alongside org query, which removes the requirement for the organizationMeta query.

upside: better api

downside: basically writing our own ORM. maybe it's time to look at knex? 😒

todo:

  • add functionality to organizationResolver so that it intuits whether it is returning one or many records

@bnchdrff bnchdrff temporarily deployed to gnl-graphql-pr-31 January 29, 2019 14:28 Inactive
@bnchdrff bnchdrff temporarily deployed to gnl-graphql-pr-31 January 29, 2019 14:37 Inactive
@bnchdrff bnchdrff temporarily deployed to gnl-graphql-pr-31 February 1, 2019 18:08 Inactive
@bnchdrff bnchdrff mentioned this pull request Feb 2, 2019
@bnchdrff bnchdrff temporarily deployed to gnl-graphql-pr-31 February 2, 2019 15:37 Inactive
Copy link
Member

@hampelm hampelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great overall.

server.ts has grown quite a bit. At the risk of duplicating graphql docs, could you add some comments narrating what is happening or otherwise outline the structure of the file?

Would it make sense to break server.ts apart a bit? Eg put resolvers into their own directory, like resolvers/organization.ts? I'm torn between being able to see the logic all in one place (so you don't have to open 5,000 files and understand each one to make one change) and having more independent pieces that are individually easier to search for and modify.

colocate model-specific graphql code with the models

use sequelize virtual fields for all special model properties (tag totals)

put utility functions in a helper module
@bnchdrff
Copy link
Member Author

bnchdrff commented Feb 3, 2019

This looks great overall.

server.ts has grown quite a bit. At the risk of duplicating graphql docs, could you add some comments narrating what is happening or otherwise outline the structure of the file?

Would it make sense to break server.ts apart a bit? Eg put resolvers into their own directory, like resolvers/organization.ts? I'm torn between being able to see the logic all in one place (so you don't have to open 5,000 files and understand each one to make one change) and having more independent pieces that are individually easier to search for and modify.

just did a major refactor - i colocated all of the model-specific graphql code into the models themselves -- this way, graphql definitions related to each model's columns and special aggregate query columns are right next to the definitions or custom queries. i think this makes more sense!

@bnchdrff
Copy link
Member Author

bnchdrff commented Feb 3, 2019

an additional refactor step for the future, maybe? create a directory for each model, and have typedefs, sequelize models, and graphql defs each have a file

@bnchdrff bnchdrff mentioned this pull request Sep 18, 2019
@bnchdrff
Copy link
Member Author

see #33

@bnchdrff bnchdrff mentioned this pull request Sep 18, 2019
@bnchdrff bnchdrff closed this Sep 18, 2019
@bnchdrff bnchdrff deleted the fancy-resolvers branch September 18, 2019 11:48
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add top-level query resource for grant and org ntee codes Ordering grants?
2 participants