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

pull common SQL adapters and helpers into reusable functions #595

Closed
wants to merge 1 commit into from

Conversation

verdverm
Copy link
Contributor

…functions

@mitjade
Copy link
Collaborator

mitjade commented Dec 24, 2017

@verdverm You are doing an amazing job here. I am doing something a bit differently in #524. That is generating sql queries with knex, based on resolverInfo. I think combining both things could resolve into something really powerful. I initially tried integrating join-monster into this kit, but it did not fit in well in all the places I needed it. knex query builder is really powerful and above all flexible enough to do what you want.

Quick intro to my approach:
In the resolver there is a forth argument info for example:

product: async (obj, args, context, info) => ...

With the help of graphql-parse-fields you can get all query fields and then I generate all the select fields and joins so that in the end you get with the help of knexnest exact data that you need.
If you are interested in what I've done, you can check selectBy in https://github.com/sysgears/apollo-universal-starter-kit/blob/cli-crud/src/server/sql/helpers.js.
To take care of one-to-many relations I needed to use graphql-resolve-batch, so I could pass in the info. But for this I need to generate a resolver to connect it all together.

Branch is currently work in progress and many things still do not yet fully work as intended.

I am officially on vacation an really promised my wife I will not do any work over the holidays. If any of this interest you we can discuss it further, but I will not really be responsive, maybe twice day at most. After the holidays I will continue on this and I can already see many things you done, that can be useful to me in what I am doing.

@verdverm
Copy link
Contributor Author

I was definitely thinking about how the query path will probably be needed in the withAuth adapter, pairing with the info comes that comes from obj/sources

@verdverm
Copy link
Contributor Author

@mitjade
Copy link
Collaborator

mitjade commented Dec 25, 2017

@verdverm Looks interesting! A bit hard to wrap my head around all the code. Will take a closer look, when I have some time.

@verdverm
Copy link
Contributor Author

I added a HOC Table based on react-table that integrates with the paging crud helper in my madd sci PR

@verdverm
Copy link
Contributor Author

I have a hard time wrapping my head around it too!

import { decamelize } from 'humps';
import { has } from 'lodash';

export function currentFilter(queryBuilder, filter) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this function is specific to user module. Why is it here, then?


uses a more advanced filtering input and processing.

See https://github.com/sysgears/apollo-universal-starter-kit/issues/569
Copy link
Member

Choose a reason for hiding this comment

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

Is it okay to send user to the PR in the docs?

@verdverm
Copy link
Contributor Author

verdverm commented Jan 2, 2018

I've upgraded this system a bunch on my mad-sci branch. Lots of other helpers for other things too. They are starting to become better together. Check the latest commit(s) over there

@mairh
Copy link
Member

mairh commented Jan 2, 2018

Wouldn't it make sense to pull all those upgrades into smaller PR? mad-sci branch is f*****g huge and kinda impossible to do a PR or make sense out of it.

@larixer larixer force-pushed the master branch 18 times, most recently from c53e4c9 to 3fa038e Compare January 20, 2018 15:51
@larixer larixer force-pushed the master branch 26 times, most recently from 22cc234 to 06fd4b4 Compare January 22, 2018 15:20
@larixer
Copy link
Member

larixer commented Feb 6, 2018

@verdverm Are you going to take into account the points I have raised in this PR to finish it and merge? See my comments under lines of your code above

@verdverm
Copy link
Contributor Author

verdverm commented Feb 9, 2018

I cut a new PR on this one too. #650

@verdverm verdverm closed this Feb 11, 2018
@larixer larixer deleted the sql-list-helpers branch October 6, 2018 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants