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: add WithHeader & WithCookie #48

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Laisky
Copy link

@Laisky Laisky commented Dec 11, 2019

can set http client's headers and cookies

cli := NewClient(
    "url",
    httpClient,
    graphql.WithCookie("cookieName", "cookieVal"),
    graphql.WithHeader("headerName", "headerVal"),
)

fully backwards-compatible.

@IamFlowZ
Copy link

IamFlowZ commented Jan 31, 2020

Will this be merged in anytime soon? Would be specifically useful for authorization.

@Laisky
Copy link
Author

Laisky commented Feb 1, 2020

@IamFlowZ you can try my fork github.com/Laisky/graphql, fully compatible with shurcool/graphql.

https://github.com/Laisky/graphql

query.go Outdated
@@ -70,7 +70,7 @@ func writeArgumentType(w io.Writer, t reflect.Type, value bool) {
default:
// Named type. E.g., "Int".
name := t.Name()
if name == "string" { // HACK: Workaround for https://github.com/shurcooL/githubv4/issues/12.
if name == "string" { // HACK: Workaround for https://github.com/shurcooL/githubv4/issues/12
Copy link

@t829702 t829702 Aug 8, 2020

Choose a reason for hiding this comment

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

Could you @Laisky not include unnecessary changes like this?

Copy link
Author

@Laisky Laisky Aug 12, 2020

Choose a reason for hiding this comment

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

done

}

// NewClient creates a GraphQL client targeting the specified GraphQL server URL.
// If httpClient is nil, then http.DefaultClient is used.
func NewClient(url string, httpClient *http.Client) *Client {
Copy link

@dhawton dhawton Aug 9, 2020

Choose a reason for hiding this comment

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

Changing a function's signature is a very bad idea for backward compatibility. It's better to create a new function and have the old call the new. A blog entry from the Go team discusses this here: https://blog.golang.org/module-compatibility

Copy link
Author

@Laisky Laisky Aug 12, 2020

Choose a reason for hiding this comment

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

just add some option args, not change the function's behavior.

Copy link

@dhawton dhawton Aug 12, 2020

Choose a reason for hiding this comment

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

Correct, you changed the function's signature. By adding args to the function, you broke backward compatibility. See the blog post from the Go team that I linked in the original message. You should never change a function's signature in a module unless you're doing a major version release with breaking changes, and even then, try to avoid it.

Copy link
Author

@Laisky Laisky Aug 12, 2020

Choose a reason for hiding this comment

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

you broke backward compatibility

it not broke the backward compatibility. I add option args, you can just ignore these args.

Choose a reason for hiding this comment

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

The change here is to add a variadic, old implementations will not break. How is this a breaking change?

Copy link

@dhawton dhawton Aug 12, 2020

Choose a reason for hiding this comment

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

Laisky, again, you changed the function's signature. Please see the provided reference on how it is a breaking change. The blog post from the Go team explains how it's a breaking change when you add args to an exported function. From the provided link "Often, breaking changes come in the form of new arguments to a function." https://blog.golang.org/module-compatibility they also cover how to add arguments to an exported function without making it a breaking change. Don't change a function's signature if you don't need to..

Copy link
Author

@Laisky Laisky Dec 4, 2020

Choose a reason for hiding this comment

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

I add an new method called NewClientWithOptions

@Laisky Laisky force-pushed the pr-headers branch 2 times, most recently from 5d5da9b to 52aa802 Compare Aug 12, 2020
@StevenACoffman
Copy link

StevenACoffman commented Dec 2, 2020

This is a great feature, but I would also prefer it if these were on a new method like NewClientWithOptions or something.

Copy link
Author

@Laisky Laisky left a comment

This is a great feature, but I would also prefer it if these were on a new method like NewClientWithOptions or something.

good idea

}

// NewClient creates a GraphQL client targeting the specified GraphQL server URL.
// If httpClient is nil, then http.DefaultClient is used.
func NewClient(url string, httpClient *http.Client) *Client {
Copy link
Author

@Laisky Laisky Dec 4, 2020

Choose a reason for hiding this comment

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

I add an new method called NewClientWithOptions

graphql.go Outdated Show resolved Hide resolved
Copy link

@StevenACoffman StevenACoffman left a comment

Thanks! I had a minor suggestion. (also, I'm not a maintainer, but would love to see this merged)

Co-authored-by: Steve Coffman <StevenACoffman@users.noreply.github.com>
@andig
Copy link

andig commented Jul 25, 2021

Not a 1:1 replacement, but #73 might also enable this use case

@khwerhahn
Copy link

khwerhahn commented Sep 27, 2021

When is this going to be merged?

@jaskirat8
Copy link

jaskirat8 commented May 23, 2022

will this be merged ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants