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 parameter to validationDidStart and parsingDidStart #1653

Closed

Conversation

cliedeman
Copy link

@cliedeman cliedeman commented Sep 12, 2018

Added context to parsingDidStart and validationDidStart. Use case is that I collect tracing information in the context object and context is currently unavailable in these 2 hooks.

I also created interfaces for most of the hook methods so that writing graphql-extensions is easier instead of duplicating all the parameters.

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

@apollo-cla
Copy link

@cliedeman: 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 the ⛲️ feature New addition or enhancement to existing solutions label Sep 12, 2018
@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Sep 12, 2018
Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Just one small nit-picky request, but otherwise I think this looks good!

@@ -31,27 +31,46 @@ export interface GraphQLResponse {
extensions?: object;
}

export interface RequestDidStartOpts<TContext> {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for breaking these interfaces out!

Can we be more explicit than *Opts and just call these *Options? Unless there's a pattern here that's being followed that I'm missing?

Copy link
Author

Choose a reason for hiding this comment

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

No reason. Updated

@abernix abernix self-assigned this Nov 14, 2018
@abernix abernix added this to the Docs Improvements milestone Nov 14, 2018
@abernix
Copy link
Member

abernix commented Nov 14, 2018

Thanks for opening this originally. We've done some pretty extensive overhaul to the request pipeline, and I believe that this should now be available in the lifecycle hooks for new request pipeline (#1795).

We're working on updating the documentation to reflect this and I've changed a couple of the labels/milestones on this PR to reflect that. I'll leave this open for now, but I think you might find that this functionality is more easily accessible as of Apollo Server 2.2.0.

@abernix
Copy link
Member

abernix commented Feb 4, 2019

Thanks for opening this originally. Following up on my previous comment, I'm going to close this PR as the context is now available to request pipeline lifecycle events via Apollo Server plugins.

If you haven't already tried using the new plugin hooks, I suggest taking a look at the documentation in #2008 (WIP!). We'll likely be deprecating the graphql-extensions API in the future since the new plugin API should be more versatile and cover more extension points.

Let us know if you run into any problems with it!

@abernix abernix closed this Feb 4, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants