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

feat(GraphQL): add filterQueryOverride to GraphQL Service #1549

Merged
merged 7 commits into from
May 29, 2024

Conversation

ghiscoding
Copy link
Owner

image

Copy link

stackblitz bot commented May 28, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

codecov bot commented May 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (3882ce1) to head (6092bdd).

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1549     +/-   ##
========================================
+ Coverage    99.8%   99.8%   +0.1%     
========================================
  Files         198     198             
  Lines       21685   21691      +6     
  Branches     7112    7253    +141     
========================================
+ Hits        21624   21630      +6     
  Misses         55      55             
  Partials        6       6             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 28, 2024

@zewa666 so I think I got a similar approach to your OData PR #1536, however 1 major thing that makes it apart is that instead of returning a string, I'm actually asking the user to return an object of the shape { field: string; operator: string; value: any; } which is what we end up with in the GraphQL query.

However, while doing this PR, I noticed that my GraphqlFilterQueryOverrideArgs is an exact copy of your OdataFilterQueryOverrideArgs, so I think it would be best if we merge them both into 1 interface which could maybe be named as BackendServiceFilterQueryOverrideArgs and because it's generic then I think you should also move it to a backend service interface (or a new file) like backendServiceOption.interface.ts. So would you mind doing that change in your PR? I can update my PR afterward by simply using that new interface.

I know you're using OData and not GraphQL, but it would still be nice if you could take a look at my PR and tell me if it makes any sense (I don't even have a GraphQL server to test with) but I followed this SO answer that inspired my approach which I think is correct: https://stackoverflow.com/a/37981802/1212166

@zewa666
Copy link
Contributor

zewa666 commented May 28, 2024

sure I'll take a look tonight. btw can you target my branch as destination for the merge? I'm genuinly interested if thats possible, but no probs I can move the files around as you mentioned anyways.

with regards to testing perhaps a quick way to ramp up a server with auto graphql features would be to look into Hasura. or if you just want to spin up a demo on top of json files perhaps https://github.com/marmelab/json-graphql-server

Copy link
Contributor

@zewa666 zewa666 left a comment

Choose a reason for hiding this comment

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

aside from the small comment above LGTM

@ghiscoding ghiscoding merged commit 2c0a493 into master May 29, 2024
8 checks passed
@ghiscoding ghiscoding deleted the feat/graphql-filter-override branch May 29, 2024 03:37
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.

None yet

2 participants