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

Allow additional Hapi route options to be passed #1384

Merged
merged 5 commits into from
Aug 14, 2018
Merged

Allow additional Hapi route options to be passed #1384

merged 5 commits into from
Aug 14, 2018

Conversation

robinvdvleuten
Copy link
Contributor

@robinvdvleuten robinvdvleuten commented Jul 20, 2018

This PR adds the functionality to pass additional route configuration options when using the plugin with Hapi. In my case for example, I needed to modify the auth options and wasn't be able to do at (unfortunately only CORS options can be passed at the moment).
It would make the current CORS option obsolete though (as you can see in the test case).

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Jul 20, 2018
@mikl
Copy link

mikl commented Jul 25, 2018

Additional point in favour of this change – with the new cors setting in 2.0.0, it doesn’t appear possible to enable the Access-Control-Allow-Credentials header, which unfortunately makes it a pain to do local development, since create-react-app and similar usually run on a separate, hot-loading dev-server, and thus CORS is needed to talk to the GraphQL server.

@maxnachlinger
Copy link
Contributor

I'm excited for this PR too :) @robinvdvleuten - it might be worth resolving the conflicts in your branch.

@robinvdvleuten
Copy link
Contributor Author

@maxnachlinger good idea, did it immediately 👍

@robinvdvleuten
Copy link
Contributor Author

It would be nice to have a look from one of the maintainers like @evans otherwise I keep fixing merge conflicts 😔

@evans evans merged commit 989481f into apollographql:master Aug 14, 2018
@evans
Copy link
Contributor

evans commented Aug 14, 2018

@robinvdvleuten Really sorry about the delay! We'll merge and publish this right away. Thank you for the PR!

@maxnachlinger Thank you for taking a look as well

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⛲️ feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants