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 context to possibly long-running methods (external API) #47

Closed
mszostok opened this issue Jun 13, 2022 · 3 comments · Fixed by #50
Closed

Add context to possibly long-running methods (external API) #47

mszostok opened this issue Jun 13, 2022 · 3 comments · Fixed by #50

Comments

@mszostok
Copy link
Contributor

Describe the feature or problem you’d like to solve

Currently, there is no option to cancel the ongoing request.

Here is a small example of pagination request where this would be helpful https://gist.github.com/mszostok/b68ff95f85d4b4ff8a27aeed56f9d3ca.

This only fetches GitHub stars, so the payload is not heavy, however if you want to fetch all issues then you have even a bigger problem.

Proposed solution

How will it benefit CLI and its users?

  • It improves UX as you can cancel executed command, and it will be released immediately instead of being unresponsive for a few seconds.

  • It reduces the quota consumption, as you won't execute next calls if not needed and also save the bandwidth as you will inform server that the client is not waiting for the response anymore.

Additional context

There are at least two options how it can be achieved:

  1. Add new methods with the WithContext suffix. For example:

    // GQLClient is the interface that wraps methods for the different types of
    // API requests that are supported by the server.
    type GQLClient interface {
      // Do executes a GraphQL query request.
      // The response is populated into the response argument.
      Do(query string, variables map[string]interface{}, response interface{}) error
    
      // DoWithContext executes a GraphQL query request.
      // The response is populated into the response argument.
      DoWithContext(ctx context.Context, query string, variables map[string]interface{}, response interface{}) error
    }

    Similar approach as Go has for http.Request. It's not a breaking change.

  2. Change all func signature and add the context.Context as a first parameter. It's a breaking change in external API.

There is also an option to add context.Context to struct but it won't work in this case and it's ugly and problematic.

If you agree with one of the provided option, I can create a dedicated PR 👍

@mislav
Copy link
Contributor

mislav commented Jun 14, 2022

@mszostok Thanks for the feature request!

I liked the option (1). @samcoe thoughts?

@samcoe
Copy link
Contributor

samcoe commented Jun 14, 2022

@mislav I was just about to comment the same thing. Option 1 sounds like the right approach here.

@mszostok
Copy link
Contributor Author

Hi,

I prepared a PR that adds context as described in Option 1, see #50

Cheers!

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 a pull request may close this issue.

3 participants