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

Add pathBuilder argument to @rest directive #70

Merged
merged 1 commit into from
Feb 19, 2018

Conversation

HeyHugo
Copy link
Contributor

@HeyHugo HeyHugo commented Feb 17, 2018

pathBuilder is a user-provided function via a graphql variable to build up the path string.

A use case it enables is having one or more optional query string parameters e.g:
/items/filter?minPrice=5&maxPrice=20
Here minPrice and maxPrice could be omitted

See proposal in #69 and the problem it solves in #67

@apollo-cla
Copy link

@HeyHugo: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@fbartho
Copy link
Collaborator

fbartho commented Feb 18, 2018

I updated master with 5kb bundlesize if you want to go-fully green before PR review. Please ping me when you're ready for review here!

@HeyHugo HeyHugo force-pushed the feature/pathBuilder branch from e4af631 to 606d511 Compare February 18, 2018 17:18
@codecov-io
Copy link

codecov-io commented Feb 18, 2018

Codecov Report

Merging #70 into master will increase coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   92.64%   92.82%   +0.18%     
==========================================
  Files           2        2              
  Lines         231      237       +6     
  Branches       80       82       +2     
==========================================
+ Hits          214      220       +6     
  Misses         17       17
Impacted Files Coverage Δ
src/restLink.ts 92.79% <100%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41f42ef...fb8483b. Read the comment docs.

@HeyHugo HeyHugo force-pushed the feature/pathBuilder branch from 606d511 to 46f4cd1 Compare February 18, 2018 17:25
@HeyHugo
Copy link
Contributor Author

HeyHugo commented Feb 18, 2018

Ok, you can review now I think @fbartho

pathBuilder is a user provided function via a graphql variable to build up the path string. A use case it enables is having one or more optional query string parameters e.g:
"/items/filter?minPrice=5&maxPrice=20"
Here minPrice and maxPrice could be omitted

See proposal in apollographql#69 and the problem it solves in apollographql#67
@HeyHugo HeyHugo force-pushed the feature/pathBuilder branch from 46f4cd1 to fb8483b Compare February 18, 2018 19:46
@fbartho fbartho self-requested a review February 19, 2018 21:28
Copy link
Collaborator

@fbartho fbartho left a comment

Choose a reason for hiding this comment

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

Tweaked some docs, and the location of the tests, and the wording of the error message thrown along with adding a bonus test. Will merge into master shortly!

@fbartho
Copy link
Collaborator

fbartho commented Feb 19, 2018

Thanks a ton @HeyHugo !

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