Skip to content

fix: context params and pass req and res in an object #1295

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

Merged
merged 12 commits into from
Jul 28, 2020

Conversation

Weakky
Copy link
Collaborator

@Weakky Weakky commented Jul 27, 2020

closes #506

  • Fixes a post apollo server integration issue regarding the context. We were no longer passing the req as first parameter, but an object with req
  • Took that opportunity to refactor and pass the res as well as requested since quite a while by users. I know we said we didn't want addToContext to become a way to create server middleware and thus the res didn't make a lot of sense. Still, we left that issue for months now and the priority is still not there, so I figured out adding it now would be best.

TODO

  • docs
    • jsdoc
    • website api
    • website guides
    • website tutorial
    • breaking changes
  • tests

BREAKING CHANGES:

  • The function passed to schema.addToContext (known as a "Context Adder") no longer has its first parameter being the request object. Instead it now receives an object (known as the "Context Adder Lens") containing the request object. In addition, on this lens, the response object can also be found. The lens exposes the request and response objects on properties req and res respectively. If you feel strongly that they should be request and response or any other thoughts you have please let us know on the issue tracker!

    before:

    schema.addToContext((req) => {
      // do something
    })

    after:

    schema.addToContext(({ req, res }}) => {
      // do something
    })

@Weakky Weakky requested a review from jasonkuhrt July 27, 2020 17:09
@jasonkuhrt
Copy link
Member

Thanks for your pragmatism. Agree developers have waited too long for improvement here. Please document the breaking change (also make it part of the commit message). When we polish the release notes we'll copy-paste it.

Copy link
Member

@jasonkuhrt jasonkuhrt left a comment

Choose a reason for hiding this comment

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

🙌

@Weakky Weakky merged commit 1df6d23 into master Jul 28, 2020
@Weakky Weakky deleted the fix/context-req-res branch July 28, 2020 16:16
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.

Add support for adding response to the context on custom Express server
2 participants