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

Pass the context along to all the extension methods #1547

Merged
merged 5 commits into from
Sep 8, 2018
Merged

Pass the context along to all the extension methods #1547

merged 5 commits into from
Sep 8, 2018

Conversation

Whoaa512
Copy link
Contributor

@Whoaa512 Whoaa512 commented Aug 16, 2018

Addresses #1343

With this change you should now be able to implement an extension like so:

class MyErrorTrackingExtension extends GraphQLExtension {
    willSendResponse(o) {
        const { context, graphqlResponse } = o

        context.trackErrors(graphqlResponse.errors)

        return o
    }
}

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

Edit by @evans :
fixes #1343
fixes #614 as the request object can be taken from context or from requestDidStart
fixes #631 ""

@apollo-cla
Copy link

@Whoaa512: 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/

@ghost ghost added ⛲️ feature New addition or enhancement to existing solutions labels Aug 16, 2018
Addresses #1343

With this change you should now be able to implement an extension like so:

```javascript
class MyErrorTrackingExtension extends GraphQLExtension {
    willSendResponse(o) {
        const { context, graphqlResponse } = o

        context.trackErrors(graphqlResponse.errors)

        return o
    }
}
```
@seromenho
Copy link

seromenho commented Aug 31, 2018

Seems that this addresses also: (they are similar and possible duplicates)
#614
#631

Whoaa512 and others added 4 commits August 31, 2018 09:04
The context shouldn't be necessary for format, parse, validation, and
execute. Format generally outputs the state of the extension. Parse and
validate don't depend on the context. Execution includes the context
argument as contextValue
@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Sep 8, 2018
@evans evans requested a review from zionts September 8, 2018 01:02
Copy link
Contributor

@zionts zionts left a comment

Choose a reason for hiding this comment

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

I'm for this change. I think we need to make a decision going forward whether we want to be opinionated about what information goes in the context or specify the things that should go into these functions separately. For example, this would include information about client awareness and extensions to the query.

@evans evans merged commit 408198e into apollographql:master Sep 8, 2018
@evans
Copy link
Contributor

evans commented Sep 8, 2018

@Whoaa512 Thank you so much for the PR!

@martijnwalraven
Copy link
Contributor

I think there may be an alternative where context is an instance property, similar to how we do this for data sources. But let's revisit this when we finalize the API with the new request pipeline.

@cliedeman
Copy link

Thanks a lot for this change, I am using to try some tracing integration and its working well so far.

Is there any reason the context object was not added to the parsingDidStart and validationDidStart methods?

@Whoaa512
Copy link
Contributor Author

@cliedeman I think it might have just been on oversight on my part. Feel free to add another PR if your needs require it.

@cliedeman
Copy link

Thanks, PR #1653 Created

@ebenoist
Copy link

This is great and will fix issues I'm having with using sentry! Whats the normal release cadence for these things?

abernix added a commit that referenced this pull request Sep 27, 2018
abernix pushed a commit that referenced this pull request Oct 10, 2018
* Pass the context along to all the extension methods

Addresses #1343

With this change you should now be able to implement an extension like so:

```javascript
class MyErrorTrackingExtension extends GraphQLExtension {
    willSendResponse(o) {
        const { context, graphqlResponse } = o

        context.trackErrors(graphqlResponse.errors)

        return o
    }
}
```

Edit by @evans :
fixes #1343
fixes #614 as the request object can be taken from context or from requestDidStart
fixes #631 ""

* Remove context from extra extension functions

The context shouldn't be necessary for format, parse, validation, and
execute. Format generally outputs the state of the extension. Parse and
validate don't depend on the context. Execution includes the context
argument as contextValue

* Move change entry under vNext
@alexbjorlig
Copy link

Can anyone provide a small example of how to use this? I have not used extensions before - where do you provide them?

@Whoaa512
Copy link
Contributor Author

@dauledk @maimike I wrote a small example of how to use this here: #1681 (comment)

@twelve17
Copy link

twelve17 commented Jun 3, 2020

@seromenho could you shed some insight as to how this issue addresses #614 ? I'm not seeing how to access the context from the formatError function.

@seromenho
Copy link

@twelve17 It doesn't expose the context to formatError but instead It does expose it to all extension methods.
Looking at this again and actually it doesn't address #614 directly, I think it is kinda of a workaround. You implement an extension get the errors and the context and do something with them.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
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
10 participants