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

handler: replaces Handler.Schema with Handler.Params (graphql.Params) #30

Closed
wants to merge 3 commits into from

Conversation

chris-ramon
Copy link
Member

@chris-ramon chris-ramon commented Oct 15, 2017

Overview

  • handler: replaces *Handler.Schema with Handler.Params (graphql.Params).
    • So we can actually send to graphql.Do(...) the params we exactly need, like Context, RootObject, etc.
  • handler_test:
    • TestHandler_Params_NilParams: removes it since we want to graphql.Do handle validation errors such us schema is nil.
    • TestHandler_Params_RootObject: adds test unit to check we are passing correctly graphql.Params.RootObject — thanks a lot @nossila 👍
  • graphiql_test: updates it to use handler.Config.Params.
  • Closes: Feature request! #1

Action required

  • Add the following within Breaking Changes section when shipping a new release including this PR:

  • Handler.Schema was replaced by Handler.Params graphql.Params:
  • Before:
h := handler.New(&handler.Config{
  Schema: testutil.StarWarsSchema,
  GraphiQL: true,
})
  • Now:
h := handler.New(&handler.Config{
  Params: graphql.Params{
    Schema: testutil.StarWarsSchema,
  },
  GraphiQL: true,
})

Test plan

  • Unit test:
  • Manual test, via GraphiQL:
    image
package main

import (
        "net/http"
        "github.com/graphql-go/handler"
        "github.com/graphql-go/graphql"
        "github.com/graphql-go/graphql/testutil"
)

func main() {
        h := handler.New(&handler.Config{
                // Before:
                // Schema: &testutil.StarWarsSchema,

                // Now:
                Params: graphql.Params{
                        Schema: testutil.StarWarsSchema,
                },

                Pretty: true,
                GraphiQL: true,
        })
        http.Handle("/graphql", h)
        http.ListenAndServe(":8080", nil)
}

@coveralls
Copy link

coveralls commented Oct 15, 2017

Coverage Status

Coverage decreased (-2.0%) to 84.375% when pulling feba169 on nossila/master into b7e0284 on master.

@lwc
Copy link

lwc commented Oct 31, 2017

Being able to set RootObject is great, but this PR currently breaks request scoped context rendering this library useless for use with typical middleware based authentication & tracing techniques.

robdaemon pushed a commit to puppetlabs/handler that referenced this pull request Jul 10, 2018
Right now, there is no way to generate a default RootObject without forking this repo entirely.

This expands on graphql-go#9 by using a generator function for the root object, and graphql-go#30 by not changing any other contracts in the handler.
@chris-ramon
Copy link
Member Author

chris-ramon commented Jul 12, 2018

Totally agree with you @lwc, thanks a lot for taking a look to this PR — closing this one in favor of: #42

@chris-ramon chris-ramon deleted the nossila/master branch July 12, 2018 01:03
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